chain: drop redundant check

[matthewhughes934]

While processing children in a chain, e.g. the 1.foo.bar in
root.1.foo.bar there was a check that would skip handling comments
(and so remove them) if the use_try_shorthand config option was set.
This is because earlier in the processing (trace: Chain::from_ast ->
Chain::make_subexpr_list -> Chain::pop_expr_chain) we would replace
try!(..) expressions with ? ones but not update spans, so things
would get confused later if we tried to use these spans to recover
comments.

However, it's not possible for a try! macro to appear as a child, e.g.
root.try!(bar);, so this check is unnecessary. There are added parser
failure tests to demonstrate this.

Aside: Of course a: try! can be the root of a chain try!(foo).bar is
perfectly valid. And ? can be used anywhere in a chain:
foo.try!(bar) is invalid, but foo.bar? is valid. This change only
focuses on the try! macros.

Fixes: #6121

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

[ytmimi]

@matthewhughes934 Not a huge deal, but since we've added the LLM disclosure checkboxes to our PR template it would be nice to see them checked on all PRs going forward.

Note that we've adopted rust-lang/rust's LLM policy

[matthewhughes934]

since we've added the LLM disclosure checkboxes to our PR template it would be nice to see them checked on all PRs going forward.

👍 restored that. I have some muscle memory from keeping the PR description in sync with commits where I'll just git log --format='%b' | xclipb locally -> edit the PR description -> highlight all -> replace, so I'll be careful to keep that trailing bit there.

Hm, triagebot has a detection/message for that.

If the commit can potentially end up in rust-lang/rust (this is what the repo syncing does I think?). Then I think I should make an effort to disambiguate it there too, but triagebot won't be able to help me with that 🥲

I've update the description/commit for both those things

[ytmimi]

The PR description didn't fully help me understand why this is a redundant check. Can you try to explain it differently?

Also, is it fine to just remove the FIXME comment?

View changes since the review

[matthewhughes934]

Here's my attempt at rewriting description/commit body to be clearer (trying to TL;DR: previously we would skip rewriting comments here if we were removing try! expressions because of how spans work. But in fact, we can never have a try! here, so we don't need to worry about it).

While processing children in a chain, e.g. the 1.foo.bar in
root.1.foo.bar there was a check that would skip handling comments
(and so remove them) if the use_try_shorthand config option was set.

This is because previously in the processing (trace: Chain::from_ast
-> Chain::make_subexpr_list -> Chain::pop_expr_chain) we would
replace try!(..) expressions with ? ones but not update spans,
making it difficult to recover comment snippets.

However, it's not possible for a try! macro to appear as a child, e.g.
root.try!(bar);. Firstly note the try! is not a valid identifier,
then the possible types of chain are:

• MethodCallExpression → Expression . PathExprSegment ( CallParams? )
  (e.g. root.some_method()): PathExprSegment starts with a
  PathIdentSegment, starts with a IDENTIFIER: try! cannot be the
  start of a PathExprSegment so we can't have try! in a method
  chain.
• FieldExpression → Expression . IDENTIFIER (e.g. root.sub): try!
  is not a valid identifier, so we can't have try! as a field
  expression
• TupleIndexingExpression → Expression . TUPLE_INDEX (e.g. root.0):
  tuple index is repeated decimal digits: try! doesn't match
• keywords:.await, .use, .yield: try! doesn't
  match

Of course a: try! can be the root of a chain try!(foo).bar is
perfectly valid. And ? can be used anywhere in a chain:
foo.try!(bar) is invalid, but foo.bar? is valid. This change only
focuses on the try! macros.

[matthewhughes934]

Quicker version: some_expr.r#try!(anything) (or some_expr.try!(anything)) is invalid syntax. So we only need to worry about this conversion (from try! to ?) when working with the parent of a chain (which is elsewhere in the function I've changed, but that has its own problems, e.g. #7101).

[ytmimi]

Quicker version: some_expr.r#try!(anything) (or some_expr.try!(anything)) is invalid syntax. So we only need to worry about this conversion (from try! to ?) when working with the parent of a chain (which is elsewhere in the function I've changed, but that has its own problems, e.g. #7101).

That's very helpful and now I get what's going on. We only need to worry about try! as the root of the chain. Just in case the parser changes sometime in the future to allow some_expr.r#try!(anything) or some_expr.try!(anything) could we add two parser failure test cases to src/test/parser.rs.

// try_macro_cant_be_chain_child
fn main() {
    some_expr.r#try!(anything)
}

// raw_try_macro_cant_be_chain_child
fn main() {
    some_expr.try!(anything)
}

[matthewhughes934]

could we add two parser failure test cases to src/test/parser.rs.

Perfect! I didn't realise those tests exists, makes things much clearer than my attempt at explaining the situation 😅 (also means I can slim down the commit message)

[ytmimi]

Okay, so now chains that wouldn't format because of comments will start formatting, is that correct?

View changes since this review

[ytmimi]

Actually, In addition to the test case in tests/target/chains.rs I think we need to create a dedicated issue_6121.rs test case where we set use_try_shorthand=true to fully validate that setting it won't prevent a chain that doesn't use try! from getting formatted.

View changes since this review

[ytmimi]

Now that I think of it I think we should break this out into it's own test case where we set use_try_shorthand=true to show that setting it won't prevent the chain from getting formatted like was mentioned in the original issue. Would also be nice to turn the example from the original issue into a test case:

fn main() {
    let e: Result<(), &str> = Ok(());
    let _e = e // hello comment
            .map_err(|_| "This is an error message") // Hello comment - Fails to format
        .map(|_| ());

    println!("Hello, world!");
}

[matthewhughes934]

yeah, why didn't I add a test case with use_try_shorthand=true lol. Added test 196dc7c.

@rustbot ready

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[ytmimi]

Adding the X-impacts-stable-formatted-code and X-impacts-stable-options label because this change has the potential to change stable chain formatting if the user has use_try_shorthand=true configured.