Skip to content

regression: proc-macro to_string change for "invisible" delimiters #97608

Description

@Mark-Simulacrum

https://crater-reports.s3.amazonaws.com/beta-1.62-1/beta-2022-05-20/gh/Patryk27.lxd-snapper/log.txt

Seems to be due to an older(?) version of indoc, possibly already fixed, but wanted to file in case there was an unintentional change on our end.

cc @dtolnay (indoc-impl author, I believe)

dtolnay/indoc@29e8c96 may have been related to this as well.

Activity

  1. added
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    on Jun 1, 2022
  2. added this to the 1.62.0 milestone on Jun 1, 2022
  3. added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jun 1, 2022
  4. changed the title [-]regression: proc-macro tokenization change for literals?[/-] [+]regression: proc-macro to_string change for "invisible" delimiters[/+] on Jun 1, 2022
  5. dtolnay commented on Jun 1, 2022

    @dtolnay
    Member

    That repo's Cargo.lock is set to indoc 1.0.3. This error is fixed in indoc 1.0.5 and a better fix (which you linked) in 1.0.6.

    It's not clear to me that @nnethercote's #96682 was intended to apply to proc_macro's Display impls, as opposed to only rustc_ast_pretty and various places that tokens get shown in diagnostics, and whether that should've had a team FCP.

  6. nnethercote commented on Jun 1, 2022

    @nnethercote
    Contributor

    cc @petrochenkov, who knows more about all this stuff than I do.

  7. dtolnay commented on Jun 1, 2022

    @dtolnay
    Member

    @rust-lang/libs-api, @rust-lang/compiler:
    #96682 has the effect (among other things) that proc_macro::TokenTree's Display impl is going to put /*«*/ /*»*/ around tokens of type TokenTree::Group with delimiter=Delimiter::None. #97076 has a minimal example.

    This broke a small number of macros which involved manipulating the Display representation of tokens. One example is this code in scale-info which is a primitive way of turning a type into a pleasantly-spaced type name string, like ( bool , bool ) -> "(bool, bool)":

    Another example is this code in indoc for categorizing kinds of literals:

    I don't necessarily object to the change but it's worth being aware that this has made the strings /*«*/ and /*»*/ part of the API of proc_macro whether we like it or not and crates are going to start parsing exactly those strings, so any future change to how Delimiter::None appears in Display output is going to become even more disruptive.

    If someone feels inclined to run a post-hoc FCP we may have time for one before 1.62 (4 weeks from now) or we can revert and take longer.

  8. nnethercote commented on Jun 1, 2022

    @nnethercote
    Contributor

    My inclination is to back out #96682 and move on, but @petrochenkov may have a better idea.

  9. petrochenkov commented on Jun 1, 2022

    @petrochenkov
    Contributor

    Yes, reverting #96682 should be fine.

  10. apiraino commented on Jun 1, 2022

    @apiraino
    Contributor

    Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

    @rustbot label -I-prioritize +P-medium

  11. added and removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Jun 1, 2022
  12. added a commit that references this issue on Jun 2, 2022
    77e1069
  13. nnethercote commented on Jun 2, 2022

    @nnethercote
    Contributor

    #97636 is open for the reversion.

  14. added 2 commits that reference this issue on Jun 2, 2022
    249ffc0
    1d71237
  15. wesleywiser commented on Jun 2, 2022

    @wesleywiser
    Member

    Re-opening to track the beta backport.

  16. the8472 commented on Jun 3, 2022

    @the8472
    Member

    This broke a small number of macros which involved manipulating the Display representation of tokens.

    We recently discussed Display stability in t-libs. One idea is to use specialization to have a ToString impl that's separate from Display. If the compiler crates emitting those macros could make use of specialization then we could point user crates to use ToString instead while leaving the delimiters in Display.

  17. added a commit that references this issue on Jun 24, 2022
    babf4fa
  18. emilyalbini commented on Jun 24, 2022

    @emilyalbini
    Member

    Being backported in #98440.

  19. workingjubilee commented on Jul 6, 2022

    @workingjubilee
    Member

    This seems to have been fully resolved for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P-mediumMedium priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.regression-from-stable-to-betaPerformance or correctness regression from stable to beta.

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions