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

Support macro None groupings #18211

Open
Veykril opened this issue Sep 30, 2024 · 2 comments
Open

Support macro None groupings #18211

Veykril opened this issue Sep 30, 2024 · 2 comments
Labels
A-macro macro expansion A-proc-macro proc macro Broken Window Bugs / technical debt to be addressed immediately C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) E-hard

Comments

@Veykril
Copy link
Member

Veykril commented Sep 30, 2024

Our macro "model" isn't perfect at handling what macros can actually do, notably we do not parse or handle "None" delimited groups today (rust-lang/rust#67062, notably rustc only partially does as well). It is not really clear what none groups even are supposed to be doing, but its generally agreed (and observable) that they basically work like parentheses in expressions (and presumably the same way in types and pattern positions). In the former, expressions, we currently emit parentheses in some cases to keep parsing precedence, but this is actually quite wrong as proc-macros can observe this difference causing issues for them when they assume a none group. Additionaly, macro fragment captures become none grouped (for the opaque captures at least), something we fail to do as well leading to rust-lang/rust#67062.

In rustc, none groups are handled by encoding them in the parse tree directly, which is easily doable by the fact that expansion expands trees into nodes (replacing the macro call node). Then the parser just has to handle the none groups appropriately. (there are still some problems rustc has here as well, partially cleaned up by rust-lang/rust#124141)

In r-a, things work a bit differently, instead of replacing macro call nodes with their expansion, we keep them, handling the expansion kind of like a virtual syntax tree (virtual in the sense that it does not belong to a file). The main issue here is that we'd need to encode none groups in the syntax tree, but that isn't nicely doable as they could appear wherever in the tree (so we can't just add a new expression kind and the like for it). This is especially annoying when it comes to changing our trivia model #6584, as with that we'd want to have a predictable syntax tree layout which wouldn't be the case with this implemented ... Discarding none groups where they don;'t really do anything is not necessarily possible either, as proc-macros could observe them. So if rustc doesn't do it, we can't really do it either

@Veykril Veykril added A-macro macro expansion A-proc-macro proc macro Broken Window Bugs / technical debt to be addressed immediately C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) E-hard labels Sep 30, 2024
@ChayimFriedman2
Copy link
Contributor

@Veykril You plan to rewrite our mbe to be more like rustc, no? I imagine this will be easier after.

@dhedey
Copy link

dhedey commented Jan 21, 2025

Hi @Veykril. Thanks for raising this issue - it sounds like a hefty one to resolve!

I just wanted to add to this ticket to explain one of the impacts here, for others looking at this area...


It's currently causing some spurious compile errors in the procedural macro which I'm creating (preinterpret), which are quite a frustrating developer experience.

From a technical perspective, it seems that None-groups which are nested in some other groups are not being round-tripped when written/read from proc_macro::TokenStream, when running in rust-analyzer.

This is causing behaviour differential and crashes/errors inside the procedural macro itself.

I may have to do quite a major rework of the logic to avoid using TokenStreams inside my macro except at entrance/exit to avoid users getting spurious errors.

Minimal Reproduction

#[proc_macro]
pub fn demonstrate_group_behaviour(input: proc_macro::TokenStream) -> proc_macro::TokenStream {
    use proc_macro::{Group, TokenStream, TokenTree, Delimiter};

    fn wrap_in_group(inner: TokenStream, delimiter: Delimiter) -> TokenStream {
        core::iter::once(TokenTree::Group(Group::new(delimiter, inner))).collect()
    }

    fn unwrap_singleton_group(input: TokenStream, delimiter: Delimiter) -> TokenStream {
        let mut iter = input.into_iter();
        let first = match iter.next() {
            Some(first) => first,
            None => panic!("Unwrapped stream was empty"),
        };
        if !iter.next().is_none() {
            panic!("Unwrapped stream was not a single group");
        }
        match first {
            TokenTree::Group(group) if group.delimiter() == delimiter => group.stream(),
            other => panic!("Expected a {:?}-delimited group, found: {:?}", delimiter, other),
        }
    }

    // This panics in rust-analyzer, if the inner delimiter is Delimiter::None because
    // rust-analyzer flattens the group out, so it can't be unwrapped
    let outer_delimiter = Delimiter::Bracket;
    let inner_delimiter = Delimiter::None;
    let wrapped = wrap_in_group(wrap_in_group(input, inner_delimiter), outer_delimiter);
    unwrap_singleton_group(unwrap_singleton_group(wrapped, outer_delimiter), inner_delimiter)
}

Test case:

  • Closing and opening VSCode triggers rust-analyzer to run and the error appears. Running the test uses rustc and the error disappears.
  • We don't get the issue if it's only a single depth. Maybe because the proc_macro buffers token streams, and only sends them to the language server once they're wrapped in a group... or maybe rust-analyzer correctly handles Groups at the top level:
#[test]
fn compare_rustc_and_rust_analyzer() {
    test_crate::demonstrate_group_behaviour!("Hello World");
}

The error message in rust-analyzer (but not rustc) is:

proc-macro panicked: Expected a None-delimited group, found: Literal { kind: Str, symbol: "Hello World", suffix: None, span: SpanData { range: 80..93, anchor: SpanAnchor(EditionedFileId(FileId(0), Edition2021), 9), ctx: SyntaxContextId(0) } }rust-analyzer[macro-error](https://rust-analyzer.github.io/manual.html#macro-error)

Example error from preinterpret

In case it's of interest, I wanted to show a concrete issue / test case which is currently giving spurious errors in my IDE:

    preinterpret!({
        [!set! #items = 0 1 2 3]
        [!string! [!intersperse! {
            items: {
                #items // #items is "grouped variable syntax" so outputs [!group! 0 1 2 3]
            },
            separator: [_],
        }]]
    });

Which gives a preinterpret error (only in rust-analyzer) of:

Expected the { ... } block to output a single [ ... ] group or transparent group. You may wish to replace the outer `{ ... }` block with a `[ ... ]` block, which outputs all its contents as a stream.

Note in preinterpret, we have #x which outputs the contents of x in a transparent group and #..x which appends the contents of x ungrouped. Having groups allows things like destructuring to work sensibly, e.g. [!let! (#x, #y) = (#a, #b)].

The main reason to use none-delimited groups instead of e.g. ()-delimited groups is so that #x can be used in other places (such as macro output) without causing issues with accidental brackets making their way into the output.

dhedey added a commit to dhedey/preinterpret that referenced this issue Jan 21, 2025
dhedey added a commit to dhedey/preinterpret that referenced this issue Jan 21, 2025
dhedey added a commit to dhedey/preinterpret that referenced this issue Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macro macro expansion A-proc-macro proc macro Broken Window Bugs / technical debt to be addressed immediately C-Architecture Big architectural things which we need to figure up-front (or suggestions for rewrites :0) ) E-hard
Projects
None yet
Development

No branches or pull requests

3 participants