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

regression: c"..." are experimental #113235

Closed
Tracked by #105723
Mark-Simulacrum opened this issue Jul 1, 2023 · 13 comments
Closed
Tracked by #105723

regression: c"..." are experimental #113235

Mark-Simulacrum opened this issue Jul 1, 2023 · 13 comments
Assignees
Labels
F-c_str_literals `#![feature(c_str_literals)]` P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jul 1, 2023

Not clear to me whether this is expected -- I don't see much in the way of commentary on the RFC, and I thought we had reserved these literals previously, but perhaps that reservation was incomplete?

https://crater-reports.s3.amazonaws.com/beta-1.71-4/beta-2023-06-25/gh/noc7c9.pyfection/log.txt

@Mark-Simulacrum Mark-Simulacrum added T-lang Relevant to the language team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jul 1, 2023
@Mark-Simulacrum Mark-Simulacrum added this to the 1.71.0 milestone Jul 1, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 1, 2023
@fmease
Copy link
Member

fmease commented Jul 1, 2023

c"…" inside macro calls (with macro matches like c$e:expr) should definitely continue to compile in editions 2015 and 2018. Those prefixes are only reserved in edition 2021 and onward.

The following reproducer compiles successfully on stable in editions 2015 and 2018 but fails to compile on beta and nightly while it should remain valid:

macro_rules! m { (c$e:expr) => {}; }

fn main() { m!(c"hello"); }
stderr
error: no rules expected the token `c"hello"`
 --> src/main.rs:3:16
  |
1 | macro_rules! m { (c$e:expr) => {}; }
  | -------------- when calling this macro
2 |
3 | fn main() { m!(c"hello"); }
  |                ^^^^^^^^ no rules expected this token in macro call
  |
note: while trying to match `c`
 --> src/main.rs:1:19
  |
1 | macro_rules! m { (c$e:expr) => {}; }
  |                   ^

error[E0658]: `c".."` literals are experimental
 --> src/main.rs:3:16
  |
3 | fn main() { m!(c"hello"); }
  |                ^^^^^^^^
  |
  = note: see issue #105723 <https://github.com/rust-lang/rust/issues/105723> for more information

@rustbot label S-has-mcve

@rustbot rustbot added the S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue label Jul 1, 2023
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 1, 2023
@fmease
Copy link
Member

fmease commented Jul 1, 2023

This would be easy to fix if we had access to the edition in rustc_lexer but I don't know if it's acceptable to pass that information to Cursor.
Alternatively, we could unravel CStr and CStrRaw literals into two tokens (Ident and Str / StrRaw) after the fact in rustc_parse::lexer where we have access to the edition, although that's a bit messier I guess (and other parsers like rust-analyzer's would need to remember this step & redo this work).

@Noratrieb Noratrieb added the F-c_str_literals `#![feature(c_str_literals)]` label Jul 1, 2023
@hameerabbasi
Copy link
Contributor

Assigning P-medium since this is only a warning, and only valid for older editions of Rust.

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 2, 2023
@fmease
Copy link
Member

fmease commented Jul 2, 2023

since this is only a warning

@hameerabbasi It's not a warning but a hard error. noc7c9/pyfection no longer compiles due to this bug. rustc lexes the crate differently than before.

@hameerabbasi
Copy link
Contributor

@fmease Do you recommend P-high? Feel free to change the label if so.

@fmease
Copy link
Member

fmease commented Jul 2, 2023

I guess the priority is fine, I just wanted to set the record straight :)

@compiler-errors compiler-errors added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Jul 2, 2023
@fmease
Copy link
Member

fmease commented Jul 2, 2023

Without doing a bisection, I can tell you that this regressed in #108801 (regressed in: 8ff3903, still a regression after refactoring commit: a49570f) when rustc_lexer/src/lib.rs was modified (manually reverting those changes to the lexer makes the reproducer compile again).

@rustbot label -E-needs-bisection

@rustbot rustbot removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Jul 2, 2023
@compiler-errors
Copy link
Member

Let's at least revert the lexer changes until @fee1-dead or someone else can fix this in a way that's compatible with the pre-2021 editions @fmease, do you want to do that, or shall I?

@fmease
Copy link
Member

fmease commented Jul 4, 2023

Just reverting the lexer change & keeping everything else related to c_str_literals? I can do that within this hour when I find the time. Edit: And reverting all the tests.

@rustbot claim

@compiler-errors
Copy link
Member

To be clear, all the existing cstring tests would break, so you'll have to mark some of the tests as // known-bug: ### and open an issue tracking that functionality getting added. That'll make it so someone knows to pick it up and fix it 😸

@fmease
Copy link
Member

fmease commented Jul 4, 2023

Slightly unrelated, but I've noticed that the GitHub label should be renamed from F-c_str_literal to F-c_str_literals and its description needs to be updated, too (see https://github.com/rust-lang/rust/blob/master/compiler/rustc_feature/src/active.rs for the source of truth).

fee1-dead added a commit to fee1-dead-contrib/rust that referenced this issue Jul 6, 2023
…=compiler-errors

Revert the lexing of `c"…"` string literals

Fixes \[after beta-backport\] rust-lang#113235.
Further progress is tracked in rust-lang#113333.

This PR *manually* reverts parts of rust-lang#108801 (since a git-revert would've been too coarse-grained & messy)
and git-reverts rust-lang#111647.

CC `@fee1-dead` (rust-lang#108801) `@klensy` (rust-lang#111647)
r? `@compiler-errors`

`@rustbot` label F-c_str_literals beta-nominated
@fmease
Copy link
Member

fmease commented Jul 12, 2023

This is now fixed on beta and can be closed, right?

@nagisa
Copy link
Member

nagisa commented Jul 16, 2023

Correct, we have an issue tracking reimplementation at #113333

@nagisa nagisa closed this as completed Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-c_str_literals `#![feature(c_str_literals)]` P-medium Medium priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants