chain: drop redundant check

[matthewhughes934]

In the code: chain_item is a child in the chain, and not the root,
e.g. 1.foo.bar in root.1.foo.bar. So the question is, can a
try!(..) expression appear there, e.g. root.try!(something).0? My
conclusion is that it cannot: firstly note try! is not a valid
identifier (because of the !), so working through the possible chain
types (see pop_expr_chain):

• MethodCallExpression → Expression . PathExprSegment ( CallParams? )
  (e.g. root.some_method()): PathExprSegment starts with a
  PathIdentSegment, starts with a IDENTIFIER: try! doesn't match
• FieldExpression → Expression . IDENTIFIER (e.g. root.sub): try!
  doesn't match
• 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.

This fixes an issue: as previously, if the use_try_shorthand config
was set we would skip over any comments in any children of the chain,
so they would be dropped. The added test case covers this situation.

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

[ytmimi]

Also, when referencing issues or PRs for this repo in commit messages it's probably best to use rust-lang/rustfmt#6121 so that this doesn't accidentally refer to a rust-lang/rust issue or PR when we sync back up with that repo. Would you mind updating the commit message?

[jieyouxu]

Also, when referencing issues or PRs for this repo in commit messages it's probably best to use rust-lang/rustfmt#6121 so that this doesn't accidentally refer to a rust-lang/rust issue or PR when we sync back up with that repo. Would you mind updating the commit message?

Hm, triagebot has a detection/message for that.

EDIT: #7098, we just didn't enable it on this repo.

[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.