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: macro expansion/resolution change of some kind #119875

Closed
Mark-Simulacrum opened this issue Jan 12, 2024 · 8 comments
Closed

regression: macro expansion/resolution change of some kind #119875

Mark-Simulacrum opened this issue Jan 12, 2024 · 8 comments
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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 Jan 12, 2024

[INFO] [stdout] error[E0424]: expected value, found module `self`
[INFO] [stdout]   --> src/capabilities/weapon.rs:13:23
[INFO] [stdout]    |
[INFO] [stdout] 13 | #[derive(Clone, Copy, CapabilityTrait)]
[INFO] [stdout]    |                       ^^^^^^^^^^^^^^^
[INFO] [stdout]    |                       |
[INFO] [stdout]    |                       `self` value is a keyword only available in methods with a `self` parameter
[INFO] [stdout]    |                       this function doesn't have a `self` parameter
[INFO] [stdout]    |
[INFO] [stdout]    = note: this error originates in the derive macro `CapabilityTrait` (in Nightly builds, run with -Z macro-backtrace for more info)
[INFO] [stdout] help: add a `self` receiver parameter to make the associated `fn` a method
[INFO] [stdout]    |
[INFO] [stdout] 13 | #[derive(Clone, Copy, &self, CapabilityTrait)]
[INFO] [stdout] error: expected one of `->`, `<`, `where`, or `{`, found `%`
[INFO] [stdout]   --> tests/smoke.rs:5:5
[INFO] [stdout]    |
[INFO] [stdout] 5  | /     repeated!(for x in [0;9;3] {
[INFO] [stdout] 6  | |         fn Welcome_%%x%%() {
[INFO] [stdout] 7  | |             repeated!(for y in [1;%%x%%;2] {
[INFO] [stdout] 8  | |                 println!("From within the macro %%x%%:%%y%%!");
[INFO] [stdout] 9  | |             });
[INFO] [stdout] 10 | |         }
[INFO] [stdout] 11 | |     });
[INFO] [stdout]    | |______^ expected one of `->`, `<`, `where`, or `{`
[INFO] [stdout]    |
[INFO] [stdout]    = note: this error originates in the macro `repeated` (in Nightly builds, run with -Z macro-backtrace for more info)
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Jan 12, 2024
@Mark-Simulacrum Mark-Simulacrum added this to the 1.76.0 milestone Jan 12, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 12, 2024
@compiler-errors
Copy link
Member

This regressed in #114571, cc @nnethercote @petrochenkov.

This breakage seems to be acknowledged here: #114571 (comment)

I'm surprised this was just r+'d rather than going through some team consensus or comment period given that it breaks crates in the wild.

@nnethercote
Copy link
Contributor

There are a small number of crates that implement proc macros by calling .to_string() on the input token stream and then doing some half-assed "parsing" (i.e. regex or substring matching) of the output. This is not the right way to do it, because it relies on the output of .to_string() having a particular form, something that isn't guaranteed.

We judged that the number of affected crates was small, and their use was small, and therefore that this was acceptable. I'm happy to listen if others disagree.

@Mark-Simulacrum Mark-Simulacrum removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Jan 12, 2024
@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 12, 2024
@wesleywiser wesleywiser added the P-high High priority label Jan 18, 2024
@wesleywiser
Copy link
Member

Reviewed during T-compiler triage. We agree with @nnethercote and @petrochenkov's judgment that the breakage here is acceptable. In particular, the documentation for impl Display for TokenTree only guarantees that the output can be converted back into a TokenTree not that the output conforms to any particular formatting with regards to whitespace.

Normally, I would suggest we open PRs to the affected crates but all of the crates mentioned in the crater report have been dormant for >3 years. As such, I don't think those PRs would be merged so I'm ok with this shipping as-is in 1.76.

nnethercote added a commit to nnethercote/rust that referenced this issue Jan 22, 2024
To expressly warn against the kind of proc macro implementation that was
broken in rust-lang#119875.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
…cs, r=petrochenkov

Document `Token{Stream,Tree}::Display` more thoroughly.

To expressly warn against the kind of proc macro implementation that was broken in rust-lang#119875.

r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 22, 2024
…cs, r=petrochenkov

Document `Token{Stream,Tree}::Display` more thoroughly.

To expressly warn against the kind of proc macro implementation that was broken in rust-lang#119875.

r? ``@petrochenkov``
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 23, 2024
Rollup merge of rust-lang#120220 - nnethercote:TokenStream-Display-docs, r=petrochenkov

Document `Token{Stream,Tree}::Display` more thoroughly.

To expressly warn against the kind of proc macro implementation that was broken in rust-lang#119875.

r? ``@petrochenkov``
@nnethercote
Copy link
Contributor

Note: in #120220 I augmented the documentation for Token{Stream,Tree}::Display to explicitly warn against the substring matching approach to implementing a proc macro.

@Mark-Simulacrum: In light of all the above discussion, can we close this issue?

@Mark-Simulacrum
Copy link
Member Author

Ah, I want to double check there's a note in the release notes but will close after that.

@Mark-Simulacrum Mark-Simulacrum closed this as not planned Won't fix, can't repro, duplicate, stale Jan 24, 2024
@apiraino apiraino removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. P-high High priority labels Jan 24, 2024
@nnethercote
Copy link
Contributor

@Mark-Simulacrum: there will be a tiny bit more similar breakage in 1.77 due to further tweaks to TokenStream pretty-printing in #120227. See this comment for details about the crater run. (Note: that's the last such change to pretty-printing that I have planned.) So it might be worth adding to the release notes for 1.77 as well?

@Mark-Simulacrum
Copy link
Member Author

Those aren't even in draft yet, but if we pick up on the breakage through crater we'll certainly do so. (Also tagged the PR so that we are more likely to see it while drafting).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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