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

document and test the precise span that triggers edition-dependent behavior #86539

Open
nikomatsakis opened this issue Jun 22, 2021 · 7 comments
Labels
A-edition-2021 Area: The 2021 edition E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

My hunch is that most of the edition-dependent features don't have a documented bit of text that we use to figure out what edition they are-- and that we don't have tests for that logic. We should clarify this and document the "edition span" in the Edition Migration Guide, and then make tests that check that it works correctly.

@nikomatsakis nikomatsakis added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-needs-mentor A-edition-2021 Area: The 2021 edition labels Jun 22, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jun 22, 2021

I did some digging through the implementations. These are the spans used by each of the edition-dependent behaviours:

  1. Prelude:
    • Not based on a single span, but on the edition of the session/crate.
  2. Panic:
    • The panic!() invocation expression, so the entire std::panic!(..) span.
  3. Lints -> hard errors
    • BARE_TRAIT_OBJECTS:
      • Span of the type (trait) that's missing dyn. E.g. T in &T.
    • ELLIPSIS_INCLUSIVE_RANGE_PATTERNS
      • Span of the ... token.
  4. Disjoint captures
    • The full span of the closure expression, so e.g. the entire |a, b| a + b span.
  5. Cargo resolver 2
    • N/A
  6. array.into_iter() exception
    • Span of the into_iter token.
  7. Or patterns
  8. Reserving syntax
    • Span of the token itself.
    • For TokenStream::from_str in a proc macro: The edition of the proc macro crate, the macro definition site.

I have no clue what happens if we somehow manage to make a std::panic!(..) or $x:pat or || a expression where the first and last tokens have different editions. (Would be very weird, but it's possible with a proc macro.) I think many diagnostics would break very badly if they point into different crates. But the metadata like the edition is probably taken from exactly one of these tokens. I'll investigate.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 22, 2021

I made a proc macro that allows changing the edition of a single token and did some experiments:

  • The span of std::panic!(..) carries the edition of just std, not of the other parts. The span of panic!(..) carries the edition of panic. It always seems to be the first token of the path. Same for :: in ::std::panic!(..). (I think it rarely matters, but we could modify this to look at the last part, so the panic token. (Or whatever name you imported it as with use std::panic as something;) Or the ! token maybe.)

  • The span of $x:pat carries the edition of just pat. Seems good, but it doesn't really work when the macro comes from another crate, due to how the workaround in Use correct edition when parsing :pat matchers #85709 works. However, I cannot reproduce this problem. Even across crates it seems to follow exactly the edition of the pat span. This is great, but I'm not sure why or how this works. (:

@m-ou-se
Copy link
Member

m-ou-se commented Jun 22, 2021

For closures:

The span of |..| body is made with lo.to(body.span), where lo is the span of the first token (move or async or static or || or |). The Span::to code has multiple FIXMEs and the behaviour when macros and multiple contexts are involved is not very obvious at all:

pub fn to(self, end: Span) -> Span {
let span_data = self.data();
let end_data = end.data();
// FIXME(jseyfried): `self.ctxt` should always equal `end.ctxt` here (cf. issue #23480).
// Return the macro span on its own to avoid weird diagnostic output. It is preferable to
// have an incomplete span than a completely nonsensical one.
if span_data.ctxt != end_data.ctxt {
if span_data.ctxt == SyntaxContext::root() {
return end;
} else if end_data.ctxt == SyntaxContext::root() {
return self;
}
// Both spans fall within a macro.
// FIXME(estebank): check if it is the *same* macro.
}

The logic of Span::to is to avoid confusing diagnostics. But its behaviour isn't really great for edition tracking.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 22, 2021

The same first_token.span.to(last_token.span) logic is used for paths like ::std::panic or &path::to::Trait for the panic and dyn edition behaviours.

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Jun 22, 2021

Impressive investigation! Note that mixing spans can also be done by macro_rules! macros, although it would be more convoluted:

// In a crate `::a`
#[macro_export]
macro_rules! mk_m {( $std:ident ) => (
    #[macro_export]
    macro_rules! m {( $panic:ident ) => ( $std::$panic!() )}
)}
// In a crate `::b`
::a::mk_m!(std);
// In a crate `::c`
m!(panic); // <- `std` is spanned at `::b`, `::` and `!` at `::a` (I think), and `panic` here, at `::c`.

@petrochenkov
Copy link
Contributor

Similar existing issue - #50122.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 22, 2021

This is an example I just posted on Zulip:

// library crate in Rust 2021:
#[macro_export]
macro_rules! x { ($t:tt) => { $t::panic!("{{"); }; }

#[macro_export]
macro_rules! y { ($t:tt) => { std::$t!("{{"); }; }


// binary crate in Rust 2018:
use dep2021::*;

macro_rules! z {
    (x) => { x!(std); };
    (y) => { y!(panic); };
}

fn main() {
    x!(std);   // 2021
    y!(panic); // 2021
    z!(x);     // 2018 !!
    z!(y);     // 2021
}

An std::panic path with either the std or panic token from a 2021 macro resolves to the 2021 panic, because Span::to prefers macros. But when both come from a macro, it just picks the first token instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2021 Area: The 2021 edition E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. 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

5 participants