Tuple indexing regression

[taiki-e]

#59421 breaks some code generated by proc macros because quote converts integer to a tokenstream with a suffix.

Proc macro:

extern crate proc_macro;
use proc_macro::TokenStream;
use quote::quote;

#[proc_macro]
pub fn foo(_: TokenStream) -> TokenStream {
    let index = 0;
    TokenStream::from(quote! {
        struct Foo(());
        fn bar() {
            let x = Foo(());
            x.#index
        }
    })
}

Calling code:

foo!();

Error:

error: suffixes on a tuple index are invalid
 --> src\lib.rs:3:1
  |
3 | foo!();
  | ^^^^^^^ invalid suffix `i32`

It can be avoided by using syn::Index::from or proc_macro::Literal::*_unsuffixed.

I have already fixed my library (taiki-e/pin-project@5f5cc35), but it can happen elsewhere.

[taiki-e]

cc @estebank

[estebank]

@petrochenkov what do you think, should we accept i32 there? It feels like a bad idea and I feel the right fix is in quote, but for expidiency we could continue accepting this suffix in particular for a couple of releases.

CC @rust-lang/compiler @rust-lang/lang @dtolnay

[Centril]

I don't think there's any language design that needs this so I'm happy with a C-future-compatibility warning here for a few releases. I agree the right fix is in quote or in dependent crates.

[dtolnay]

I believe quote is behaving correctly so I won't be making any code change associated with this in quote.

[Centril]

@dtolnay Perhaps a change in documentation pointing out the right way to achieve what the user wants?

[dtolnay]

Sure, that could be good. I filed dtolnay/quote#101 to follow up. Thanks!

[estebank]

After an in person conversations with other members of the team, I feel that the right course if action is to wait until the beta crater run and depending on the extent of the in the wild breakage, backporting a beta only change that makes this a warning instead of a hard error for one release. Although it is bad that it was allowed for a while in stable, the new behavior is indeed the right behavior. If crater doesn't catch too many crates with problems we should go ahead and leave this as is

[pnkfelix]

triage: Marking P-high for now, but maybe team will convince me it is P-medium.

[pnkfelix]

there was somewhat heated discussion in the T-compiler meeting today about this.

@nikomatsakis agreed to "try to lead it to resolution"

assigning to them.

[pnkfelix]

(in the short term, we may want to revert #59421 and backport the revert to beta.)

[Centril]

I will agree to reverting if and only if it is agreed that it will eventually become a hard error. I'm not OK with adding garbage into the grammar and I cannot say for certain that it won't conflict with future language design. Moreover, I don't want to set this as a precedent for adding other types of garbage the next time rustc has a bug and diverges from the grammar. (And this is a reminder why wg-grammar work is important... rustc should not be the spec, only an implementation among others).

[varkor]

Could we make this into a warning on beta instead of reverting completely on master? It should be a very simple fix for users and it seems very undesirable to have this bug existing for a long period of time.

[estebank]

I'm onboard with @varkor's view, possibly silencing the case of foo.0i32/foo.0u32/foo.0isize/foo.0usize which are the only "valid" cases we've seen in the wild.

---

To clarify, I believe we should:

• warn on foo.0i32/foo.0u32/foo.0isize/foo.0usize cases for a full release
  • add a note to the warning mentioning either this ticket or an explanation on how to fix quote uses
• reject anything else
• reject foo.0i32/foo.0u32/foo.0isize/foo.0usize one or two releases after incorporating the warning

[eddyb]

We can record whether the literal was created by a proc macro, through e.g. Literal::usize_suffixed, and warn for that, but error for literals parsed from source.
(if this is too much effort, we could also try to get the source of the literal Span and check that it matches the literal identically)

[nikomatsakis]

I agree with @estebank's proposal to start with a warning. I am not sure what I think the final steps are, though I think moving to a hard error eventually is probably acceptable (possibly over an edition, possibly not).

[Centril]

I don't think this warrants edition differences; those should be used for intentional shifts that are actually breaking changes... Not for fixing bugs. If you use edition differences then it is still part of the grammar spec of older editions forever.

[nikomatsakis]

@Centril I understand your opinion, I just don't know if I fully agree or not. I'd prefer to discuss that in more depth elsewhere -- when it comes to this specific issue, though, it seems that we agree that we should begin with a warning. (Which is our standard bug fix procedure.)

EDIT: Just to elaborate a bit -- this issue happens to come at a time when I'm thinking a lot about what our positioning on forwards compatibility ought to be. As the use of Rust grows, I think it behooves us to be increasingly careful about the way that we impact users. My hunch is that indeed transitioning to error makes sense here, but I think there might be cases where we have unexpected -- but long standing -- behavior that we should simply "deprecate" and not "remove".

[Centril]

@nikomatsakis So to clarify, my agreement to using the standard bug fix procedure and to land #60186 is conditional on that we agree that it is a bug and commit to making it a hard error (on all editions). I don't want to find out later that due to unclear communication or whatnot that we don't make it into a hard error.

An initial crater run has already been done #60138, and it suggests that the number of regressions are small and not systemic.

[Centril]

EDIT: Just to elaborate a bit -- this issue happens to come at a time when I'm thinking a lot about what our positioning on forwards compatibility ought to be. As the use of Rust grows, I think it behooves us to be increasingly careful about the way that we impact users.

Sure; I'm thinking about it as well but I take the opposite view. I think we already have a really strict policy around breakage and if we make it more strict then I fear we will drown under the baggage of things that come with it.

While this specific issue has no soundness implications, I think we must value soundness more than we value stability. I also think the same of adding garbage to the grammar but less strongly. As Rust becomes standardized, it does not seem fair that the maintainers of rustc can introduce garbage into the grammatical specification via bugs. In my view, we need to take specificability seriously and that includes not having assorted amounts of hacks in the language which were introduced due to bugs or for other reasons.

[varkor]

Perhaps these discussions are indicative that Rust has grown to the point where having a single source of truth (i.e. rustc) is damaging to the language. If there were multiple implementations of Rust, it would not make sense to have a discussion about whether to accept a bug that has slipped through during implementation, because it wouldn't be compliant with other implementations (or, rather, the language reference itself).

Recently there have been discussions about better references for the Rust language and I think that separating the implementation from the design is very important at this stage. If we are very concerned with making sure we don't break anyone, then perhaps we need to focus on developing better mitigations (e.g. a watertight rustfix for past compiler bugs) rather than accepting past mistakes.

[Centril]

@varkor Yeah totally. Focusing on specification is something that is important and that we should work on this year. It just takes a long time and a whole lot of work to actually do it.

[nikomatsakis]

I think that this is clearly unintentional behavior. As such, it should be treated like a bug, which means that it should start with a warning which will eventually become an error (this is our standard procedure for these sorts of situations). @Centril, I found this comment rather frustrating:

my agreement to using the standard bug fix procedure and to land #60186 is conditional on that we agree that it is a bug and commit to making it a hard error (on all editions)

In particular, I don't think that you need to "agree" to use the standard procedure. What would need to be justified is going straight to a hard error.

However, I appreciate the motivation of this later comment:

I don't want to find out later that due to unclear communication or whatnot that we don't make it into a hard error.

I realize I am sending some mixed signals here. This is in part because I don't quite know what to think. So let me clear it up. I am willing to agree that we should treat this case as an error and move to a hard error after some time (on all editions). I agree that the limited crater run suggests we can do this safely, and I don't think there is any question that the existing behavior is a bug.

However, I still think we need to have a broader conversation on the topic of our backwards compatibility procedure. If nothing else, I'd like to see the compiler team triaging and handling future-compatibility warnings more carefully, and us adopting standard time frames (e.g., N releases or what have you).

While I of course would like to see us make progress on a specification, I don't actually think it's the high order bit here. In this case, we all agree this is not what a specification would say, if one existed. It often happens that compilers accept some older behavior for reasons of backwards compatibility. I also think it's plausible that a specification might make space to accommodate the legacy behavior of rustc, particularly in older editions. It depends on what kinds of things we encounter.

I don't think this particular bug needs to be such a time, thankfully.

[Centril]

@nikomatsakis

I realize I am sending some mixed signals here.

I apologize if I sounded harsh, but this is mainly why I wrote that.

I am willing to agree that we should treat this case as an error and move to a hard error after some time (on all editions). I agree that the limited crater run suggests we can do this safely, and I don't think there is any question that the existing behavior is a bug.

Great, that's precisely the clarification I wanted. :) Seems we are agreed then.

If nothing else, I'd like to see the compiler team triaging and handling future-compatibility warnings more carefully, and us adopting standard time frames (e.g., N releases or what have you).

I do strongly agree with standard time frames (ideally adjustable based on number of regressions) and in particular, I think scheduled checkups are a good idea. Having #59658 work (especially the --cap-lints part) is important partly to actually make the warnings have effect so that N is reasonably small.

I also think it's plausible that a specification might make space to accommodate the legacy behavior of rustc, particularly in older editions. It depends on what kinds of things we encounter.

Sure. I agree in theory that some rustc bugs may warrant embracing into the specification. #[deprecated = "foobar"] and #[should_panic = "foobar"] are examples of this. In those cases I found it reasonable to retcon it into the language since a) it was harmless and easy to support, b) the newly supported attribute form makes sense and could be justified independently of "we made an oops", c) it did not have consequences for soundness, and d) it did not introduce any new syntax by virtue of using already existing syntax in the language.

I don't think this particular bug needs to be such a time, thankfully.

Yeah, agreed.

[pnkfelix]

closing this ticket as the regression has been addressed (PR #60186) and backported to beta. #60210 tracks the future-compat warning which will eventually turn into a hard error.