Skip to content

chain: drop redundant check #7095

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.

Sign up for GitHub

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

Open
matthewhughes934 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chain: drop redundant check #7095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/chains.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,7 @@ impl Chain {
}
while let Some(chain_item) = iter.next() {
let comment_snippet = context.snippet(chain_item.span);
// FIXME: Figure out the way to get a correct span when converting `try!` to `?`.
let handle_comment =
!(context.config.use_try_shorthand() || is_tries(comment_snippet.trim()));
let handle_comment = !is_tries(comment_snippet.trim());

@ytmimi ytmimi Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 matthewhughes934 Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Pre-comment
if handle_comment {
Expand Down
6 changes: 6 additions & 0 deletions tests/source/chains.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,3 +264,9 @@ fn issue_3034() {
disallowed_headers.iter().any(|header| *header == name) ||
disallowed_header_prefixes.iter().any(|prefix| name.starts_with(prefix))
}

fn issue_6121() {
let _ = foo // some comment
.bar // some other comment
.buz;
}
6 changes: 6 additions & 0 deletions tests/target/chains.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,3 +304,9 @@ fn issue_3034() {
disallowed_headers.iter().any(|header| *header == name)
|| disallowed_header_prefixes.iter().any(|prefix| name.starts_with(prefix))
}

fn issue_6121() {
let _ = foo // some comment
.bar // some other comment
.buz;
}