Make Literal::byte_character_value work with bytes as well

[GuillaumeGomez]

View all comments

As noted in this comment, byte_character_value should work for bytes, so this PR fixes it.

r? @traviscross

[GuillaumeGomez]

Just realized the failure is because of the GCC backend. Not sure why, gonna ignore this test for now for the GCC backend.

[GuillaumeGomez]

CI is happy now.

[traviscross]

Suggested change

	bridge::LitKind::Char | bridge::LitKind::Byte => {
	bridge::LitKind::Byte => {

What's the explanation for checking for both rather than just for Byte? And what error would we expect character('é').byte_character_value() to return?

View changes since the review

[GuillaumeGomez]

'A' is a valid byte (like all ascii characters). As for the error, I expect ConversionErrorKind::InvalidLiteralKind.

[traviscross]

As for the error, I expect ConversionErrorKind::InvalidLiteralKind.

I'm getting Err(FailedToUnescape(NonAsciiCharInByte)) instead.

(Let's please assert the correct answer in the test rather than just checking .is_err().)

@rustbot author

[GuillaumeGomez]

Which is even better, gonna update the test though to reflect it.

[traviscross]

'A' is a valid byte (like all ascii characters).

We don't accept:

let _:u8 = 'A';//~ ERROR mismatched types

The documentation says this function:

Returns the unescaped character value if the current literal is a byte character literal.

The behavior I would therefore expect here is:

#![feature(proc_macro_value, rustc_private)]externcrate proc_macro;use proc_macro::{Literal,TokenStream,TokenTree,ConversionErrorKind::*,EscapeError::*};#[proc_macro]pubfndemo(input:TokenStream) -> TokenStream{let c = r"b'A'".parse::<Literal>().unwrap();assert_eq!(c.character_value(),Err(InvalidLiteralKind));assert_eq!(c.byte_character_value(),Ok(65));let c = r"'A'".parse::<Literal>().unwrap();assert_eq!(c.character_value(),Ok('A'));assert_eq!(c.byte_character_value(),Err(InvalidLiteralKind));"()".parse().unwrap()}

Thoughts?

[GuillaumeGomez]

My base argument for all this is that we already allow to get the value between integer/float types even if the type doesn't match exactly. So I think it's acceptable in this case to be a bit more flexible.

[traviscross]

I'm thinking about what you said about integer/float types. Regarding your example:

But this code is valid:

fnmain(){let x:u8 = 'a'as_;let y:char = b'a'as_;println!("{x:?} {y:?}");}

Note that this branch does not accept this in the opposite direction. I.e., it conforms to this behavior:

#[proc_macro]pubfndemo(input:TokenStream) -> TokenStream{let c = r"b'A'".parse::<Literal>().unwrap();assert_eq!(c.character_value(),Err(InvalidLiteralKind));assert_eq!(c.byte_character_value(),Ok(65));let c = r"'A'".parse::<Literal>().unwrap();assert_eq!(c.character_value(),Ok('A'));assert_eq!(c.byte_character_value(),Ok(65));"()".parse().unwrap()}

[GuillaumeGomez]

Good point. Should I allow it to match integer/float types then?

[traviscross]

No, I wouldn't think so. We're operating at a syntactic level here. I would not expect the availability of as casts or other later-stage conversions to affect what we accept at this level. I'd expect byte_character_value to return Ok(…) only for byte character literals.

In a similar way, in #154608, what you implemented conformed to this behavior:

let c = r"0u8".parse::<Literal>().unwrap();assert_eq!(c.u8_value(),Ok(0));assert_eq!(c.u16_value(),Err(InvalidLiteralKind));assert_eq!(c.f32_value(),Err(InvalidLiteralKind));let c = r"0u16".parse::<Literal>().unwrap();assert_eq!(c.u8_value(),Err(InvalidLiteralKind));assert_eq!(c.u16_value(),Ok(0));assert_eq!(c.f32_value(),Err(InvalidLiteralKind));let c = r"0".parse::<Literal>().unwrap();assert_eq!(c.u8_value(),Ok(0));assert_eq!(c.u16_value(),Ok(0));assert_eq!(c.f32_value(),Err(InvalidLiteralKind));let c = r"0.".parse::<Literal>().unwrap();assert_eq!(c.u8_value(),Err(InvalidLiteralKind));assert_eq!(c.u16_value(),Err(InvalidLiteralKind));assert_eq!(c.f32_value(),Ok(0.));

That seems correct to me. u8_value() accepts 0u8 and 0 as those are both valid u8 literals. u16_value() accepts 0u16 and 0 as those are both valid u16 literals. But u16_value() does not accept 0u8 even though we can as cast from it to a value of type u16. And f32_value() doesn't accept any of the integer literals.

My base argument for all this is that we already allow to get the value between integer/float types even if the type doesn't match exactly.

I'm not certain that I understand the argument that you're making here. If I didn't capture it above, I'm curious to know, so please do elaborate.

[GuillaumeGomez]

Fair enough.

[traviscross]

@rustbot author

[GuillaumeGomez]

@rustbot ready

[GuillaumeGomez]

It's strange that we need this one:

error[E0658]: use of unstable library feature `rustc_private`: this crate is being loaded from the sysroot, an unstable location; did you mean to load this crate from crates.io via `Cargo.toml` instead?
--> /home/imperio/rust/rust/tests/ui/proc-macro/auxiliary/api/literal.rs:3:18
|
LL | use proc_macro::{EscapeError, Literal, ConversionErrorKind};
| ^^^^^^^^^^^
|
= note: see issue #27812 <https://github.com/rust-lang/rust/issues/27812> for more information
= help: add `#![feature(rustc_private)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

Because the reexport is done as follow in proc-macro:

#[unstable(feature = "proc_macro_value", issue = "136652")]pubuse rustc_literal_escaper::EscapeError;

I suppose it's because it comes from a dependency, but that remains surprising.

View changes since the review

[Mark-Simulacrum]

That seems like we probably don't want to re-export it and instead have a wrapper around EscapeError? Obviously we'll want to clean this up before stabilization, at least to avoid stability implications in rustc-literal-escaper, but seems OK for here.

[GuillaumeGomez]

@rustbot ready

[GuillaumeGomez]

Fixed merge conflict.

[GuillaumeGomez]

Updated.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Mark-Simulacrum]

That seems like we probably don't want to re-export it and instead have a wrapper around EscapeError? Obviously we'll want to clean this up before stabilization, at least to avoid stability implications in rustc-literal-escaper, but seems OK for here.

[traviscross]

@bors r=traviscross,Mark-Simulacrum rollup

(@Mark-Simulacrum and I are talking about this on the lang-docs call.)

[rust-bors]

📌 Commit e27598e has been approved by traviscross,Mark-Simulacrum

It is now in the queue for this repository.

[GuillaumeGomez]

@Mark-Simulacrum: You mean like #157442? 😉