Improve print_tts by making space_between smarter

[nnethercote]

space_between currently handles a few cases that make the output nicer. It also gets some cases wrong. This PR fixes the wrong cases, and adds a bunch of extra cases, resulting in prettier output. E.g. these lines:

use smallvec :: SmallVec ;
assert! (mem :: size_of :: < T > () != 0) ;

become these lines:

use smallvec::SmallVec;
assert!(mem::size_of:: < T >() != 0);

This overlaps with #114571, but this PR has the crucial characteristic of giving the same results for all token streams, including those generated by proc macros. For that reason I think it's worth having even if/when #114571 is merged. It's also nice that this PR's improvements can be obtained by modifying only space_between.

r? @petrochenkov

[petrochenkov]

This PR changes punctuation jointness in many cases, something I advised against in the previous similar PR - #97340 (comment).

After #114571 lands and after Spacing::Unspecified is added for proc-macro generated non-punctuation tokens space_between heuristic will only be taken into account for Spacing::Unspecified tokens.

So, right now, I think, that would be equivalent to never using space_between if tt1 is a punctuation token.
@rustbot author

[petrochenkov]

So, right now, I think, that would be equivalent to never using space_between if tt1 is a punctuation token.

Update: when both tt1 and tt2 are punctuation tokens.
The pretty-printing result is a text, and it will be processed by lexer if treated as Rust code, so the jointness will be erased anyway if tt2 is not a punctuation token.

[nnethercote]

Ok, I have updated the code to never remove the space between adjacent punctuation tokens. The following cases are worse:

• #![attr] becomes # ![attr]
• use A::*; becomes use A:: * ;
• < Self as T >::thing becomes < Self as T > ::thing
• Struct::< u8 > becomes Struct:: < u8 >
• f(x: Vec<u32>, u32) becomes f(x: Vec<u32> , u32)

but overall it's not too bad, and still a lot better than the current output (though #! [attr] becoming # ![attr] is annoying).

The existing space_between gets the "must have space between adjacent tokens" rule wrong for some cases involving ; and $ and , and !. So I have also added some more tests to stringify.rs containing just sequences of operators. The updated space_between gets all these cases right.

I ended up merging all the previous commits that change the behaviour of space_between.

[petrochenkov]

I don't think this function should return anything in this case, it should rather have assert!(!is_punct(tt1) || !is_punct(tt2)) at the start instead.

The decision should be made based on Spacing in that case, and we should never reach this function.

[petrochenkov]

Or the Spacing-based decision can be made inside this function, then it will be if is_punct(tt1) && is_punct(tt2) { ... } at the start instead of the assert.

[nnethercote]

I would prefer to avoid making any pretty-printing decisions based on Spacing in this PR. We can leave those to #114571, which will change how space_between is called. I plan to add the Spacing::Unknown in that PR, for tokens coming from proc macros. Those will be the cases where space_between is used.

With that decided, the current position of the assertion has the advantage that it's only checked in the case where space_between returns false.

So I think this is good enough to merge, or do a crater run if you think that is necessary.

[petrochenkov]

Ok.
Crater run is needed in any case.

[petrochenkov]

These examples still involve punctuation and need an update.

[petrochenkov]

@bors try

[bors]

⌛ Trying commit 9b9f8f0 with merge 794be3c...

[bors]

☀️ Try build successful - checks-actions
Build commit: 794be3c (794be3cb2f20e1fd15af31d8a42d380fc36600ed)

[petrochenkov]

@craterbot check

[craterbot]

👌 Experiment pr-117433 created and queued.
🤖 Automatically detected try build 794be3c
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-117433 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[petrochenkov]

@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-117433/retry-regressed-list.txt

[craterbot]

👌 Experiment pr-117433-1 created and queued.
🤖 Automatically detected try build 794be3c
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-117433-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-117433-1 is completed!
📊 36 regressed and 0 fixed (53 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[petrochenkov]

There are some legitimate regressions here, waiting on author to triage.
@rustbot author

[nnethercote]

🎉 Experiment pr-117433-1 is completed! 📊 36 regressed and 0 fixed (53 total) 📰 Open the full report.

I still don't understand how to read crater reports properly. But looking at the "regressed: dependencies" section, there are five problems, all involving conversions of a token stream to a string and then "parsing" the resulting string. Annoying stuff, and I'm not sure how to proceed.

atspi-proxies-0.1.0

It uses atspi-macros-0.4.0, which has this code in src/proxy.rs:

// TODO: this is sketchy as all hell 
// it replaces all mentions of zbus::Result with the Generic std::result::Result, then, adds the Self::Error error type to the second part of the generic
// finally, it replaces all mentions of (String, zbus :: zvairnat :: OwnedObjectPath) with &Self.
// this menas that implementors will need to return a borrowed value of the same type to comply with the type system.
// unsure if this will hold up over time.
fn genericize_method_return_type(rt: &ReturnType) -> TokenStream {
        let original = format!("{}", rt.to_token_stream());
        let mut generic_result = original.replace("zbus :: Result", "std :: result :: Result");
        let end_of_str = generic_result.len();
        generic_result.insert_str(end_of_str - 2, ", Self :: Error");
        let mut generic_impl = generic_result.replace(OBJECT_PAIR_NAME, "Self");
        generic_impl.push_str(" where Self: Sized");
        TokenStream::from_str(&generic_impl).expect("Could not genericize zbus method/property/signal. Attempted to turn \"{generic_result}\" into a TokenStream.")
}

The spacing of zbus :: Result has changed to zbus::Result, which breaks this code.

awto-0.1.2

It uses awto-macros-0.1.2, which has this code in src/proc_macros/schema/database_table.rs:

                let db_type_is_text = ty.to_string().ends_with(":: Text");
                if let Some(max_len) = &field.attrs.max_len {
                    if !db_type_is_text {
                        return Err(syn::Error::new(
                            max_len.span(),
                            "max_len can only be used on varchar & char types",
                        ));
                    }
                    ty = quote!(#ty(Some(#max_len)));
                } else if db_type_is_text {
                    ty = quote!(#ty(None));
                }

The change in spacing of :: Text to ::Text breaks this.

ink-analyzer-ir-0.7.0

crates/macro/src/from_ast.rs has this:

pub fn impl_from_ast(ast: &syn::DeriveInput) -> syn::Result<TokenStream> {                                                                            
    let name = &ast.ident;                                                                                                                            
                                                                                                                                                      
    if let Some(fields) = utils::parse_struct_fields(ast) {                                                                                           
        if let Some(ast_field) = utils::find_field(fields, "ast") {                                                                                   
            let ir_crate_path = utils::get_normalized_ir_crate_path();                                                                                
            let ast_field_type = &ast_field.ty;                                                                                                       
            let ast_type = if ast_field_type                                                                                                          
                .to_token_stream()                                                                                                                    
                .to_string()                                                                                                                          
                .starts_with("ast ::")
            {                                                                                                                                         
                quote! { #ast_field_type }                                                                                                            
            } else {                                                                                                                                  
                quote! { ast::#ast_field_type }                                                                                                       
            };  

The change of spacing from ast :: to ast:: breaks this.

kcl-lib-0.1.35

It uses derive-docs-0.4.0 which has this code in src/lib.rs:

    let ret_ty = ast.sig.output.clone();
    let ret_ty_string = ret_ty
        .into_token_stream()
        .to_string()
        .replace("-> ", "")
        .replace("Result < ", "")
        .replace(", KclError >", "");
    let return_type = if !ret_ty_string.is_empty() {
        let ret_ty_string = if ret_ty_string.starts_with("Box <") {
            ret_ty_string
                .trim_start_matches("Box <")
                .trim_end_matches('>')
                .trim()
                .to_string()
        } else {
            ret_ty_string.trim().to_string()
        };

An example return type changes from this:

-> Result < Box < ExtrudeGroup >, KclError > {}

to this:

-> Result < Box < ExtrudeGroup > , KclError > {}

And the space inserted between the > and , breaks the parsing, because trim_end_matches('>') fails due to the trailing space.

pagetop-0.0.46

This uses pagetop-macros-0.0.5, which has this code in in src/lib.rs:

    let args: Vec<String> = fn_item                                                                                                  
        .sig                                                                                                                         
        .inputs                                                                                                                      
        .iter()                                                                                                                      
        .skip(1)                                                                                                                     
        .map(|arg| arg.to_token_stream().to_string())                                                                                
        .collect();                                                                                                                  
                                                                                                                                     
    let param: Vec<String> = args                                                                                                    
        .iter()                                                                                                                      
        .map(|arg| arg.split_whitespace().next().unwrap().to_string())                                                               
        .collect();                                                                                                                  
                                                                                                                                     
    #[rustfmt::skip]                                                                                                                 
    let fn_with = parse_str::<ItemFn>(concat_string!("                                                                               
        pub fn ", fn_name.replace("alter_", "with_"), "(mut self, ", args.join(", "), ") -> Self {                                   
            self.", fn_name, "(", param.join(", "), ");                                                                              
            self                                                                                                                     
        }                                                                                                                            
    ").as_str()).unwrap();                                                                                                           

On a signature like this:

    pub fn alter_value(&mut self, value: impl Into<String>) -> &mut Self {                                                           

the old pretty printer printed value :, the new one does value:, so the arg.split_whitespace().next() returns value: instead of value, which messes up the subsequent code.

[petrochenkov]

I think we can follow the usual breaking change procedure here.

If the regressed crate is alive - send a fix.
Stuff like contains("foo :: bar") can be trivially replaced with contains("foo :: bar") || contains("foo::bar").

If the regressed crate is also popular (e.g. responsible for the largest number of regressions in the report), then special case it in the compiler.
E.g. do not remove spaces around :: if the neighboring tokens are foo and bar.

@rustbot author

[nnethercote]

Interestingly, the joint-based pretty printer in #114571 doesn't break the five examples above. Presumably because the affected token streams come from proc macros, and #114571 doesn't make changes to how those are printed.

Now that I better understand the effects of these kinds of changes on real world code, I have the following plan:

• Merge the first two commits from this PR in Clarify space_between #117698, because they're useful and make no functional changes.
• Give up on improving space_between, or scale it back to just the changes that don't cause errors.
• Return to the joint-based approach in Improve print_tts #114571.

[nnethercote]

Now that I better understand the effects of these kinds of changes on real world code, I have the following plan:

• Merge the first two commits from this PR in Clarify space_between #117698, because they're useful and make no functional changes.
• Give up on improving space_between, or scale it back to just the changes that don't cause errors.
• Return to the joint-based approach in Improve print_tts #114571.

These have all been done.