Add new Literal::str_value, Literal::cstr_value and Literal::byte_str_value methods
#504
Conversation
dtolnay
left a comment
dtolnay
left a comment
There was a problem hiding this comment.
Thanks for the PR. Since the API of these methods is likely to change, in order to move this forward they need to be behind the procmacro2_semver_exempt cfg like the unstable Span methods such as def_site — a Cargo feature is not appropriate for this. Separately, each method needs a fallback implementation with identical behavior that works in non-macro code.
|
Makes sense! A bit busy currently but I'll try to come back to this as soon as possible. |
GuillaumeGomez
force-pushed
the
new-literal-api
branch
2 times, most recently
from
3381417 to
c40fc5e
Compare
|
Removed the feature and switched to the |
GuillaumeGomez
force-pushed
the
new-literal-api
branch
from
c40fc5e to
bf98d28
Compare
…e_str_value` methods
GuillaumeGomez
force-pushed
the
new-literal-api
branch
from
bf98d28 to
88592ad
Compare
|
Do I need to create a new |
| unicode-ident = "1.0" | ||
|
|
||
| [target.'cfg(procmacro2_semver_exempt)'.dependencies] | ||
| rustc-literal-escaper = "0.0.5" |
There was a problem hiding this comment.
This dependency's compiler version support is not compatible with making it a dependency of proc-macro2. Also it has unreasonably many publishers for something that would become a widely used dependency. Neither of these factors is necessarily immediately disqualifying while the dependency is gated by our semver exempt cfg, but they mean that this implementation is not going to work once it comes time to stabilize the new methods. I would prefer going directly to a different implementation that does not involve adding this dependency.
| } | ||
|
|
||
| /// Returns the unescaped string value if the current literal is a string or a string literal. | ||
| #[cfg(all(procmacro2_semver_exempt, feature = "proc-macro"))] |
There was a problem hiding this comment.
This cannot be conditional on feature = "proc-macro". We need to provide methods that work identically whether or not proc-macro is being used.
| Literal::_new(unsafe { imp::Literal::from_str_unchecked(repr) }) | ||
| } | ||
|
|
||
| /// Returns the unescaped string value if the current literal is a string or a string literal. |
There was a problem hiding this comment.
It is not clear to me what distinction this is making between a literal that is a string vs a string literal.
| match self.inner { | ||
| imp::Literal::Compiler(ref compiler_lit) => compiler_lit.byte_str_value(), | ||
| imp::Literal::Fallback(ref fallback) => { | ||
| if !fallback.repr.starts_with('c') { |
There was a problem hiding this comment.
Wrong prefix for a byte string.
Please add a test that would catch this.
| error = Some(ConversionErrorKind::FailedToUnescape(unsafe { | ||
| std::mem::transmute(err) |
There was a problem hiding this comment.
This unsafe transmute is unsound. In general the version of rustc-literal-escaper in the proc-macro2 user's dependency graph is going to be different than the version of rustc-literal-escaper compiled into the standard library.
| } | ||
| let mut error = None; | ||
| let mut buf = String::with_capacity(fallback.repr.len()); | ||
| rustc_literal_escaper::unescape_str(&fallback.repr, |_, res| match res { |
There was a problem hiding this comment.
According to the documentation of this function it "takes the contents of a string literal (without quotes)".
Please add a test that would catch this.
| if !fallback.repr.starts_with('"') { | ||
| return Err(ConversionErrorKind::InvalidLiteralKind); |
There was a problem hiding this comment.
Is this the intended behavior on raw string literals?
| #![cfg_attr(docsrs, feature(doc_cfg))] | ||
| #
2 participants
Since rust-lang/rust#139367 was merged a while ago, I think it's ok to start making use of it as a nightly experiment if you're ok with it.
If this PR is merged, I plan to send a follow-up on
synforLiteral::valueto make use of these methods instead (whennightlyfeature is enabled) so the unescaping source code could becfg-ed out.Hopefully, when the API gets stabilized, we could get some nice numbers on compilation improvements for a lot of crates (although minor, considering how much your crates are used, definitely worth it).
So what do you think?