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

Rename try! macro for 2018 compatibility #1385

Closed
withoutboats opened this issue Sep 12, 2018 · 10 comments
Closed

Rename try! macro for 2018 compatibility #1385

withoutboats opened this issue Sep 12, 2018 · 10 comments
Labels

Comments

@withoutboats
Copy link

serde derive uses a custom defined try! macro internally, rather than ?. However, try is becoming a reserved word in the 2018 edition. For compatibility with 2018, it would be ideal if you renamed this macro.

@Mark-Simulacrum
Copy link

Hm, it's not clear to me that Serde should be responsible for making this change? That is, when/if Serde migrates to 2018 edition then it should but prior to that it was my understanding that edition hygiene should not require this -- am I wrong about that?

@withoutboats
Copy link
Author

You're correct that users should not get edition related warnings from the expansion of external macros.

@Mark-Simulacrum
Copy link

I'm not quite sure what the intent of this issue is then since I can't seem to produce any warnings -- is this just a long-term cleanup?

@dtolnay
Copy link
Member

dtolnay commented Sep 12, 2018

I'm not clear then. Is this something we need to prioritize before the edition? Or is this just tracking work that needs to happen at some point before we change our own edition to 2018, which may be years later?

@withoutboats
Copy link
Author

According to the user report on internals, the compiler might not be successfully performing edition hygiene in all cases. Obviously, this is a bug in the compiler. But this seems like a small change to serde_derive that would help mitigate edition hygiene bugs and keep them from impacting the many users of this crate.

@dtolnay
Copy link
Member

dtolnay commented Sep 13, 2018

Internals thread: Trying Rust2018, getting many warnings on `try` is a keyword

I am not able to reproduce the warnings reported there. If anyone succeeds in reproducing and can explain why this is something that a large number of users would encounter, then I can reconsider, but for now I am closing this issue. Thanks anyway! We'll take care of this when moving Serde to 2018 but don't need an issue to track this specific piece of that work.

Hopefully this helps track down the remaining places were edition hygiene is going wrong. I believe our use of try is within the space of what needs to be supported.

@dtolnay dtolnay closed this as completed Sep 13, 2018
@dtolnay dtolnay added the wontfix label Nov 9, 2018
jean-airoldie added a commit to jean-airoldie/serde that referenced this issue Nov 6, 2019
jean-airoldie added a commit to jean-airoldie/serde that referenced this issue Nov 6, 2019
jean-airoldie added a commit to jean-airoldie/serde that referenced this issue Nov 6, 2019
@alisianoi
Copy link

alisianoi commented Jun 2, 2020

If anyone succeeds in reproducing and can explain why this is something that a large number of users would encounter, then I can reconsider, but for now I am closing this issue.

For me there is a warning in my emacs. My configuration (relevant part is lines 80 to 90) is inspired by a fairly common configuration for Rust development from reddit. The warning is triggered whenever I save the file, my screen gets split in two halves and the bottom half shows the exception (twice for whatever reason):

Screenshot from 2020-06-02 19-15-15

If this is theoretically just an issue of replacing ~200 occurances of try! in the codebase, I can do that. So far, I've replaced all try! occurances in two files and cargo test still works:

  1. serde/src/ser/mod.rs
  2. serde/src/ser/impls.rs

My Rust skills are limited, so I actually have no idea if replacing try! with ? will break things

Edit:

% cargo --version
cargo 1.45.0-nightly (cb06cb269 2020-05-08)
% rustc --version
rustc 1.45.0-nightly (a74d1862d 2020-05-14)

@dtolnay
Copy link
Member

dtolnay commented Jun 2, 2020

That looks like the emacs integration is building the code using the wrong edition. Is there a bug report about that?

@alisianoi
Copy link

At my previous comment, my rustup toolchain list looked like this:

nightly-2019-08-27-x86_64-unknown-linux-gnu
nightly-2020-02-27-x86_64-unknown-linux-gnu
nightly-2020-04-08-x86_64-unknown-linux-gnu
nightly-x86_64-unknown-linux-gnu (default)

I removed every toolchain with rustup toolchain uninstall and then reinstalled nightly like so:

$ rustup toolchain install nightly
$ rustup toolchain list
nightly-x86_64-unknown-linux-gnu (default)

I also removed all of my rust-related .emacs configurations and only left the "run rustfmt when I press save" configuration and restarted my emacs:

(setq rust-format-on-save t)                                                                                                                                   

With that, the error message changed slightly but is still largely the same:

Screenshot from 2020-06-03 00-29-52

@dtolnay
Copy link
Member

dtolnay commented Jun 2, 2020

I will lock this issue for now because this isn't the place to debug that particular emacs bug.

@serde-rs serde-rs locked and limited conversation to collaborators Jun 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

4 participants