Skip to content

fix: Don't trigger postfix completion in if block which has an else block #14123

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

Merged
merged 2 commits into from
Feb 14, 2023
Merged

fix: Don't trigger postfix completion in if block which has an else block #14123

merged 2 commits into from
Feb 14, 2023

Conversation

dqkqd
Copy link
Contributor

@dqkqd dqkqd commented Feb 11, 2023

Fix #14096

Discard postfix completion if the next_non_trivia_sibling of dot_token
is an ELSE_KW.
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 11, 2023
Some(ast::Expr::IfExpr(_)) => {
let next_sibling = field.dot_token().and_then(|token| {
let dot_token = original_file.covering_element(token.text_range());
let next_sibling = dot_token.as_token().and_then(|t| t.next_token()).and_then(|t| next_non_trivia_sibling(t.into()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to find the next sibling by calling next_non_trivia_sibling on dot_token, but this return None.

dot_token.as_token().and_then(|t| next_non_trivia_sibling(t.into()))

So I find the next_non_trivia_sibling on dot_token.next_token() instead. I'm not sure if this is 100% correct.

Copy link
Member

Choose a reason for hiding this comment

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

There is no need to fetch the dot token from the original file, this logic here can be done fully with the fake syntax tree. The reason why next_non_trivia_sibling fails is because the else keyword is wrapped in an error node that is the sibling of the parent of the field expression.

If we use the fake file instead I think it should be enough to just check for the next non trivia token (next from the fake NameRef that we get passed in this function. Then we also don't need those next_sibling branches that check for a node that contains else, we can just check the else token.

Copy link
Contributor Author

@dqkqd dqkqd Feb 13, 2023

Choose a reason for hiding this comment

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

I updated it, but I found no method to find next_non_trivia_token, so I wrote one based on previous_non_triva_token.

fn previous_non_trivia_token(e: impl Into<SyntaxElement>) -> Option<SyntaxToken> {
let mut token = match e.into() {
SyntaxElement::Node(n) => n.first_token()?,

I'm not sure if I should reuse next_non_trivia_sibling in this case.

fn next_non_trivia_sibling(ele: SyntaxElement) -> Option<SyntaxElement> {
let mut e = ele.next_sibling_or_token();
while let Some(inner) = e {

By the way, I think next_non_trivia_sibling is duplicated with syntax::algo::next_non_trivia_sibling.

@Veykril Veykril changed the title Don't trigger postfix completion in if block which has an else block fix: Don't trigger postfix completion in if block which has an else block Feb 13, 2023
@dqkqd dqkqd requested a review from Veykril February 13, 2023 13:24
@Veykril Veykril removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2023
@lnicola
Copy link
Member

lnicola commented Feb 14, 2023

@bors r=Veykril

@bors
Copy link
Contributor

bors commented Feb 14, 2023

📌 Commit 0285acc has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 14, 2023

⌛ Testing commit 0285acc with merge 3812951...

@bors
Copy link
Contributor

bors commented Feb 14, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 3812951 to master...

@bors bors merged commit 3812951 into rust-lang:master Feb 14, 2023
@dqkqd dqkqd deleted the discard-postfix-completion-for-indivisble-expr branch February 14, 2023 12:35
@Masynchin
Copy link

Can next_non_trivia_token have this implementation?

fn next_non_trivia_token(e: impl Into<SyntaxElement>) -> Option<SyntaxToken> {
    let token = match e.into() {
        SyntaxElement::Node(n) => n.last_token()?,
        SyntaxElement::Token(t) => t,
    }.next_token();

    iter::successors(token, Token::next_token)
         .find(|token| !token.kind().is_trivia())
}

iter::successors eliminates the need to use mutable local token and while-let loop.

@lnicola
Copy link
Member

lnicola commented Feb 24, 2023

@Masynchin it can, but we don't necessarily prefer the functional style, and the function is pretty similar to the one below (next_non_trivia_sibling).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postfix completions trigger on block expressions that are part of other expressions
6 participants