apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.62k stars 840 forks source link

php editor : infinte semicolon while autocomplet on "return" #3805

Closed eglic closed 2 years ago

eglic commented 2 years ago

Apache NetBeans version

Apache NetBeans 13

What happened

from version 12.6 to 13.0:

  1. create or open any php file
  2. input "ret" anywhere
  3. wait autocomplete menu popuped
  4. press "Enter" to select only option "return ... Language Construct"
  5. netbeans insert "return ;;;;;;;;"
  6. the character ";" will auto append infinity until press any key

How to reproduce

  1. create or open any php file
  2. input "ret" anywhere
  3. wait autocomplete menu popuped
  4. press "Enter" to select only option "return ... Language Construct"
  5. netbeans insert "return ;;;;;;;;"
  6. the character ";" will auto append infinity until press any key

Did this work correctly in an earlier version?

Apache NetBeans 12.6

Operating System

microsoft windows 11

JDK

jdk-17.0.2 -x64

Apache NetBeans packaging

Apache NetBeans binary zip

Anything else

No response

Are you willing to submit a pull request?

No

Code of Conduct

Yes

fpiccinali commented 2 years ago

Issue comfirmed also on Kubuntu 20.04.

Product Version: Apache NetBeans IDE 12.6
Java: 11.0.14; OpenJDK 64-Bit Server VM 11.0.14+9-Ubuntu-0ubuntu2.20.04
Runtime: OpenJDK Runtime Environment 11.0.14+9-Ubuntu-0ubuntu2.20.04
System: Linux version 5.4.0-105-generic running on amd64; UTF-8; fr_FR (nb)
Chris2011 commented 2 years ago

Yeah but for me it happened with this steps:

Chris2011 commented 2 years ago

If you look closer there is this in the code completion "return ;" so there is a space between return and the last semicolon. I believe that there is somehow a problem that after the space the code completion comes up again to write the correct return value but the space will not added. Just guessing.

junichi11 commented 2 years ago

Reproducible. Thank you for reporting it.

junichi11 commented 2 years ago

Related to this: https://github.com/apache/netbeans/pull/3290

@mbien Could you please have a look at this?

mbien commented 2 years ago

@junichi11 i can try, but i don't know a lot about the PHP editor.

junichi11 commented 2 years ago

@mbien Maybe, I don't think this is related to the PHP editor although I could be wrong. It seems that CC is invoked infinitely with #3290.

This problem doesn't occur after I comment-out that change.

mbien commented 2 years ago

@junichi11 as I said, I take a look but all the PR did was to show the completion again under certain conditions. It reverted parts of https://github.com/apache/netbeans/commit/a9871c5085106f1ffd6c1c7ec594774185cf69d6. The question is: why is ";" a valid completion?

mbien commented 2 years ago

what is the expected behavior? it should complete ret into return? With or without semicolon?

junichi11 commented 2 years ago

The question is: why is ";" a valid completion?

Because CC item uses the following template: https://github.com/apache/netbeans/blob/ea62da490f933d1a4d51abd53dcbf9956035ecaf/php/php.editor/src/org/netbeans/modules/php/editor/completion/PHPCompletionItem.java#L2035-L2055

I realized another problem. Maybe this case is also invoked CC again. (Popup window is shown again after we typed the enter key.) This problem also doesn't occur after I comment-out that change. netbeans-3805-cc

interface A {}
class Something implements A{
}
junichi11 commented 2 years ago

what is the expected behavior? it should complete ret into return? With or without semicolon?

Exprected: With semicolon(return^;) :^ means the caret netbeans-gh-3805-correct

Actual: netbeans-gh-3805-incorrect

mbien commented 2 years ago

return; is inserted, after that, the next completion is ";" for some reason. Since its the only completion it is automatically inserted without showing the menu, right after return, this never ends since the situation does not change.

You can replicate the same issue without #3290, just keep pressing ctrl+space, it will keep inserting ";".

some thoughts: is the cursor position correct? should it be before the semicolon? The java editor places it behind the semicolon if there is no parameter, if there is a return paramenter, it does not set the semicolon, just a space after return. Both situations "finish" completions and don't cause infinite loops.

junichi11 commented 2 years ago

is the cursor position correct?

Correct. User may return a value: return $somethig;

mbien commented 2 years ago

in that case it should not add the semicolon yet, just a space after return. If there is no param it should add return; and put the cursor behind ;, otherwise the cycle won't end.

The problem here is that the completion does not change the situation, you can keep pressing ctrl+space and get the same completion automatically inserted since the semicolon is on the right side of the cursor and does not affect the result.

java for comparison (| marks caret after completion):

    private static void foo() {
        return;|    // ret -> "return;"
    }
    private static boolean bar() {
        return |   // ret -> "return "
    }
junichi11 commented 2 years ago

The problem here is that the completion does not change the situation, you can keep pressing ctrl+space and get the same completion automatically inserted since the semicolon is on the right side of the cursor and does not affect the result.

It's a user's behavior. I think the problem is CC is invoked automatically again...

@tmysik What do you think?

tmysik commented 2 years ago

Uff, really hard to say... will try to look into this problem a bit later.

junichi11 commented 2 years ago

PHP is not Java...

// the following function may return a value or may not return anything
// PHP can declare functions/methods without return types
function foo() {
    return 1;
    // return;
}

I don't think NetBeans is IDE for only Java... We should fix the editor without breaking the existing behavior of other languages.

mbien commented 2 years ago

PHP is not Java...

indeed, but we can try to learn from it. PHP functions don't contain sufficient information in the declaration to allow the IDE to fully complete the return statement. An incomplete return statement is just like the bar method https://github.com/apache/netbeans/issues/3805#issuecomment-1139684802, (just without the space since the IDE can't guess that for PHP). The java editor does not add the semicolon there, the PHP editor probably shouldn't either. Maybe there should be two completion items for the return keyword.

But that is just an idea, as i mentioned twice already, I have limited knowledge of the PHP editor.

I don't think NetBeans is IDE for only Java...

correct. Who claimed anything else?

We should fix the editor without breaking the existing behavior of other languages.

i have a draft pr pending which mitigates the symptom a bit in situations when completions are not behaving well (e.g if they keep looping when pressing ctrl+space). But i have to test a bit more to make sure it doesn't affect other languages (yes, other than PHP and Java ;)) and more complex templates. Essentially having to test that the behavior of NB <12.2 is still there and also that #2519 works (without causing the regression added by the same PR).

junichi11 commented 2 years ago

I hope that this problem is fixed in the Editor. It has worked for a long time as the expected behavior in the PHP editor until that change is included.

The java editor does not add the semicolon there, the PHP editor probably shouldn't either.

"PHP is not Java..." and "I don't think NetBeans is IDE for only Java..." mean this... Please imagine, when I fix something for the PHP feature in the Editor, the Java editor feature is broken. What do you think if I say "The PHP editor does not ... , the Java editor shouldn't ...."?

tmysik commented 2 years ago

@junichi11 @mbien

I just read all the comments here, and I think that you both are right. I mean, the change in the editor caused this unwanted behavior in the PHP support, however, perhaps the PHP CC could be improved (but we have no idea, how many other plugins could be also broken by this change; personally, I expect not many).

Junichi, I quite like this idea:

Maybe there should be two completion items for the return keyword.

One for return;${cursor} and one for return ${invokeCompletion}

What do you think? Do you see any possible problem if we change it?

junichi11 commented 2 years ago

how many other plugins could be also broken by this change

My concern is this. Invoking CC infinitely is risky, I think... The client-side does not know anything about whether the template has a problem. So, we can use unexpected templates for the current implementation of the editor accidentally easily.

@mbien I guess we should handle it in the Java editor. The Java editor should know the context. So, please try to invoke CC again from the Java editor if needed instead of fixing the editor.

What do you think? Do you see any possible problem if we change it?

Probably, there is no problem with this case(return), I suppose. However, at least, we have a problem other than this case(return). (The popup window is not hidden until we press the ESC key.) We have to investigate the other cases whether it works correctly. I want to avoid it... As I wrote above, maybe this editor's change is risky because the template usage is restricted.

mbien commented 2 years ago

you can help testing #4165 if you want.

junichi11 commented 2 years ago

I was misunderstanding. I thought @mbien is fixing a Java issue, sorry.

junichi11 commented 2 years ago

I've verified this doesn't occur with #4165.

mbien commented 2 years ago

I've verified this doesn't occur with #4165.

awesome. Thanks for confirming.

mbien commented 2 years ago

going to merge the PR soon which will automatically close this issue too since they were linked.

This won't fix the php completion for "return" itself (if you keep pressing ctrl+space the editor will keep inserting ";". It basically removes the main symptom which automated that loop :) The remaining issue is probably fairly minor - I don't think many users will notice.