PHP: removed built-in type real #7387
Conversation
|
@mbien Can I ask for the tests to be restarted? I don't have the ability to set the label before creating the PR. |
php/php.editor/src/org/netbeans/modules/php/editor/parser/astnodes/CastExpression.java
Outdated
Show resolved
Hide resolved
| @@ -29,7 +29,7 @@ public class Scalar extends Expression { | |||
|
|
|||
| public enum Type { | |||
| INT, // 'int' | |||
| REAL, // 'real' | |||
| FLOAT, // 'float' | |||
There was a problem hiding this comment.
I don't think we should change this because this is an API.
There was a problem hiding this comment.
Right, we cannot do this if it is an API/SPI. If you really want to do that, the steps should be like:
- in release X
- deprecate the
REALtype + document all the details (why deprecated and when it will be removed) - add new
FLOATtype - change the code under our control to start using the
FLOATtype - increase the minor API version of the module
- deprecate the
- in release X + Y (Y is quite a big number, we cannot do it in the near future, we need to give time to our API consumers - plugins - to change their code):
- remove the
REALtype - increase the major API version of the module
- remove the
There was a problem hiding this comment.
@tmysik Thank you very much for the explanation.
Updated the PR:
- removed the
realtype removal in the public api. - increased the minor version of the module.
I hope I did everything correctly.
|
Note: If we already did something similar in #5294, then it is unfortunate, as I explained above. Ideally, we should revert the API change and follow the steps I described. OTOH, not sure how many plugins exist that are affected by this change and therefore can be broken. Also, the fix should be easy, but we must avoid of such API changes, definitely. CC @junichi11 |
troizet
force-pushed
the
php_remove_built_in_type_real
branch
from
64bb08e to
4b884fc
Compare
By similarity to #5294, I was referring to the purpose of that PR. |
php/php.editor/src/org/netbeans/modules/php/editor/parser/astnodes/CastExpression.java
Show resolved
Hide resolved
php/php.editor/src/org/netbeans/modules/php/editor/typinghooks/PhpCommentGenerator.java
Show resolved
Hide resolved
| @@ -156,7 +156,7 @@ public void visit(Scalar node) { | |||
| if (CancelSupport.getDefault().isCancelled()) { | |||
| return; | |||
| } | |||
| if (node.getScalarType().equals(Scalar.Type.REAL) && node.getStringValue().startsWith(BINARY_PREFIX)) { | |||
| if (node.getScalarType().equals(Scalar.Type.FLOAT) && node.getStringValue().startsWith(BINARY_PREFIX)) { | |||
There was a problem hiding this comment.
@junichi11 Again, please, verify that we can simply replace REAL -> FLOAT and not keep the both types. Thank you.
There was a problem hiding this comment.
It should be OK (I assume all REAL types were replaced with FLOAT in the parser).
But I would keep both types, just in case.
There was a problem hiding this comment.
The only use case would be if we had it in the index, right? Otherwise, we should be safe.
But as we agreed, I believe that keeping both types should be super-safe. Once we remove the REAL type, it will be very easy to delete all its usages.
@troizet, could you, please, update this PR? Thanks for understanding, and sorry for any inconveniences, we are trying to be really safe here.
There was a problem hiding this comment.
Approved, but please wait for the approval by @junichi11. Thank you!
php/php.editor/src/org/netbeans/modules/php/editor/parser/astnodes/Scalar.java
Show resolved
Hide resolved
There was a problem hiding this comment.
See the comments - please, keep the REAL type and add the FLOAT one. Later on, we will remove the REAL type with all its usages.
CC @junichi11
- deleted the built-in type real
(php does not have a built-in real type: https://www.php.net/manual/en/language.types.intro.php
Also see the warning under "Scalar Types": https://www.php.net/manual/en/language.types.declarations.php)
- fixed broken tests
- new tests added
- fixes generation of return type real instead of float for a method when overriding a base class method with an autocomplete
- fixes generation of return type real instead of float for a function in phpDoc
troizet
force-pushed
the
php_remove_built_in_type_real
branch
from
4b884fc to
a8c9cd8
Compare
|
Made the suggested corrections. |
|
@junichi11 Forgot to set milestone for this PR, or is it optional? Thank you! |
| public void testFunctionGuessingFloatReturnType_01() throws Exception { | ||
| checkCompletionDocumentation("testfiles/completion/documentation/functionGuessingFloatReturnType.php", "testFloatReturn^Type_01();", false, ""); | ||
| } |
There was a problem hiding this comment.
this test started failing recently on linux. Could this be some kind of race condition where it is sometimes returning a different completion item?
it works fine on windows.
There was a problem hiding this comment.
I also noticed it. I'll fix it ASAP. Thanks.
There was a problem hiding this comment.
It should be fixed in #7553. Sorry for the inconvenience.
There was a problem hiding this comment.
this was a bit of a dejavu for me but I can't remember where this happened before (groovy? javascript? php?)
what do you think about this change:
diff --git a/ide/csl.api/test/unit/src/org/netbeans/modules/csl/api/test/CslTestBase.java b/ide/csl.api/test/unit/src/org/netbeans/modules/csl/api/test/CslTestBase.java
index 4b21823..8eff017 100644
--- a/ide/csl.api/test/unit/src/org/netbeans/modules/csl/api/test/CslTestBase.java
+++ b/ide/csl.api/test/unit/src/org/netbeans/modules/csl/api/test/CslTestBase.java
@@ -107,6 +107,7 @@
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import javax.swing.JEditorPane;
import javax.swing.event.DocumentEvent;
import javax.swing.event.DocumentEvent.ElementChange;
@@ -182,6 +183,7 @@
import org.openide.util.Pair;
import org.openide.util.lookup.Lookups;
import org.openide.util.test.MockLookup;
+
import static org.openide.util.test.MockLookup.setLookup;
/**
@@ -3145,13 +3147,12 @@
CodeCompletionResult completionResult = cc.complete(context);
List<CompletionProposal> proposals = completionResult.getItems();
- CompletionProposal match = null;
- for (CompletionProposal proposal : proposals) {
- if (proposal.getName().startsWith(itemPrefix)) {
- match = proposal;
- break;
- }
- }
+ List<CompletionProposal> matched = proposals.stream()
+ .filter(p -> p.getName().startsWith(itemPrefix))
+ .collect(Collectors.toList());
+ assertEquals("more than one item matched the prefix", 1, matched.size());
+ CompletionProposal match = matched.get(0);
+
assertNotNull(match);
assertNotNull(match.getElement());
this would prevent this situation to re-occur, however, this would also cause many other test failures right now.
There was a problem hiding this comment.
@mbien @junichi11 I apologize for the problematic tests. Couldn't this also be related to the #6066?
There was a problem hiding this comment.
@mbien matched.size() may not be 1.
There was a problem hiding this comment.
@troizet Maybe, It is not related to this.
There was a problem hiding this comment.
@mbien
matched.size()may not be1.
in that case it needs to be sorted so that it doesn't randomly fail
There was a problem hiding this comment.
Yes, the cause is it, I think.
The main purpose of this PR is to fix the situation where NetBeans generates a
realtype and uses it as a native return type.In this PR:
real(php does not have a built-inrealtype: https://www.php.net/manual/en/language.types.intro.php Also see the warning under "Scalar Types": https://www.php.net/manual/en/language.types.declarations.php)realinstead offloatfor a method when overriding a base class method with an autocompleterealinstead offloatfor a function in phpDocThis PR is similar to #5294.