Strings cannot be nil inspection #2871

Alioramus · 2017-02-19T10:19:21Z

fixes #2144

ignatov · 2017-02-19T10:52:05Z

@Alioramus Could you please sign https://www.jetbrains.com/agreements/cla/ first.

Alioramus · 2017-02-19T10:58:25Z

Done.

zolotov

Tests are required.

Also please cover with tests all exceptions that I found in the PR.

zolotov · 2017-02-20T16:22:39Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+  protected GoVisitor buildGoVisitor(@NotNull ProblemsHolder holder, @NotNull LocalInspectionToolSession session) {
+    return new GoVisitor(){
+
+


zolotov · 2017-02-20T16:23:22Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+      @Override
+      public void visitVarSpec(@NotNull GoVarSpec o) {
+        super.visitVarSpec(o);
+        for(int i = 0; i < o.getExpressionList().size(); ++i){


Use foreach on VarDefinitions instead and use getValue method to retrieve an expression

zolotov · 2017-02-20T16:24:13Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+      public void visitVarSpec(@NotNull GoVarSpec o) {
+        super.visitVarSpec(o);
+        for(int i = 0; i < o.getExpressionList().size(); ++i){
+          check(o.getVarDefinitionList().get(i).getGoType(null), o.getVarDefinitionList().get(i).getValue());


A lot of possible NPEs

zolotov · 2017-02-20T16:26:58Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+      @Override
+      public void visitAssignOp(@NotNull GoAssignOp o) {
+        super.visitAssignOp(o);
+        PsiElement[] right = o.getParent().getChildren();


looks cryptic and non-reliable, you cannot say what children of parent are. if you want to visit some particular elements, you should visit them, not their children.

Also getParent might be null, so yet another NPE here.

zolotov · 2017-02-20T16:27:08Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+        PsiElement[] right = o.getParent().getChildren();
+        if(right == null) return;
+
+        List<GoExpression> left = ((GoLeftHandExprList)right[0]).getExpressionList();


possible ClassCastException and IndexOutOfBoundException

zolotov · 2017-02-20T16:30:42Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+        if(var == null || value == null) return;
+
+        if(GoTypeUtil.isString(var)) {
+          if(value.getText().equals(GoConstants.NIL)){


if variable's text is nil, it doesn't mean that it's really nil. It can be reference to some other variable: https://play.golang.org/p/AcZdk3hfzU

use com.goide.psi.impl.GoExpressionUtil#isNil instead

zolotov · 2017-02-20T16:32:03Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+
+        for(int i = 0; i < left.size(); ++i){
+          GoExpression var = left.get(i);
+          PsiElement value = right[right.length - (left.size() - i)];


OutOfBoundsException

zolotov · 2017-02-20T16:32:15Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+          PsiElement value = right[right.length - (left.size() - i)];
+
+          if(value instanceof GoExpression){
+            check(var.getGoType(null), (GoExpression)value);


NPE on var

zolotov · 2017-02-20T16:33:16Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+        if(GoTypeUtil.isString(var)) {
+          if(value.getText().equals(GoConstants.NIL)){
+
+            holder.registerProblem(value, PROBLEM_DESC, new LocalQuickFix() {


Please do not shorten names, PROBLEM_DESCRIPTION is just fine

zolotov · 2017-02-20T16:33:55Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+
+              @Override
+              public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
+                value.replace(GoElementFactory.createExpression(project, DEFAULT_STRING));


Memory leak on value, element from ProblemDescriptor should be used

zolotov · 2017-02-26T17:24:24Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+        PsiReference valueRef = value.getReference();
+        if(valueRef == null) return;
+        PsiElement valueResolved = valueRef.resolve();
+        if(value.textMatches(GoConstants.NIL) && valueResolved != null && GoPsiImplUtil.builtin(valueResolved)){


com.goide.psi.impl.GoExpressionUtil#isNil

){ – code style

@zolotov What do you mean by com.goide.psi.impl.GoExpressionUtil#isNil? I havent seen isNil there.

You're right, I looked at the wrong repo. Ok then, it's ok to have this here. You can optimize it though:

check that value is GoReferenceExpressions, other elements cannot be resolved to nil

check text first, it's much simpler than resolving

boolean isNil = value instanceof GoReferenceExpression && value.textMatches(GoConstants.NIL) && GoPsiImplUtil.builtin(((GoReferenceExpression)value).resolve())

zolotov · 2017-02-26T17:24:57Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+      }
+
+      protected void check(GoTypeOwner var, GoExpression value){
+


zolotov · 2017-02-26T17:25:34Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+
+      protected void check(GoTypeOwner var, GoExpression value){
+
+        if(var == null || value == null) return;


if( code style.

Please just run code formatting

zolotov · 2017-02-26T17:25:48Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+  }
+
+  public static class GoChangeStringToDefaultValueQuickFix extends LocalQuickFixBase {
+


zolotov · 2017-02-26T17:26:59Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+    public void applyFix(@NotNull Project project, @NotNull ProblemDescriptor descriptor) {
+      PsiElement element = descriptor.getPsiElement();
+      if(element == null) return;
+      element.replace(GoElementFactory.createExpression(project, DEFAULT_STRING));


element can be invalid at this point. Also you don't check what element it is. E.g. in case of batch inspection mode that was run on injected text, you'll get a file here as an element, I don't think you want to replace file with ""

zolotov · 2017-02-26T17:34:44Z

tests/com/goide/inspections/GoStringCannotBeNilInspectionTest.java

+    myFixture.testHighlighting(getTestName(true) + ".go");
+  }
+
+


zolotov · 2017-02-26T17:35:04Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+import java.util.List;
+import java.util.stream.IntStream;
+
+public class GoStringCannotBeNilInspection extends GoInspectionBase{


zolotov · 2017-02-26T17:35:11Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+
+public class GoStringCannotBeNilInspection extends GoInspectionBase{
+  public static final String QUICK_FIX_NAME = "Change to default value";
+  public static final String DEFAULT_STRING = "\"\"";


zolotov · 2017-02-26T17:35:15Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+public class GoStringCannotBeNilInspection extends GoInspectionBase{
+  public static final String QUICK_FIX_NAME = "Change to default value";
+  public static final String DEFAULT_STRING = "\"\"";
+  public static final String PROBLEM_DESCRIPTION = "String cannot be nil";


zolotov · 2017-02-26T17:37:02Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+    };
+  }
+
+  public static class GoChangeStringToDefaultValueQuickFix extends LocalQuickFixBase {


Alioramus · 2017-02-26T18:36:47Z

I hope it will be ok now and I apologise for so many mistakes.

zolotov

It looks good, just make sure that inspection file is "green" and IDE doesn't complain on it.

zolotov · 2017-02-28T11:20:23Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+      public void visitVarSpec(@NotNull GoVarSpec o) {
+        super.visitVarSpec(o);
+
+        if (o.getVarDefinitionList() == null) return;


always false

zolotov · 2017-02-28T11:20:44Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+      public void visitAssignmentStatement(@NotNull GoAssignmentStatement o) {
+        super.visitAssignmentStatement(o);
+
+        if (o.getLeftHandExprList() == null) return;


always false

zolotov · 2017-02-28T11:20:50Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+        if (o.getLeftHandExprList() == null) return;
+        List<GoExpression> rightSide = o.getExpressionList();
+        List<GoExpression> leftSide = o.getLeftHandExprList().getExpressionList();
+        if (leftSide == null || rightSide == null) return;


always false

zolotov · 2017-02-28T11:20:58Z

src/com/goide/inspections/GoStringCannotBeNilInspection.java

+        }
+      }
+
+      protected void check(GoTypeOwner var, GoExpression value) {


why protected?

zolotov · 2017-02-28T11:30:33Z

The whole inspection is questionable. According to Go language reference (https://golang.org/ref/spec#Assignability), you can assign nil only to a pointer, function, slice, map, channel, or interface type. Is it supposed to add the inspection for each type that is not pointer, function, slice, map, channel, or interface?

zolotov · 2017-02-28T11:31:15Z

Rules for comparisons: https://golang.org/ref/spec#Comparison_operators

zolotov self-assigned this Feb 19, 2017

zolotov requested changes Feb 20, 2017

View reviewed changes

Alioramus force-pushed the 2144 branch from 842fe81 to b692011 Compare February 26, 2017 15:23

zolotov requested changes Feb 26, 2017

View reviewed changes

Strings cannot be nil inspection

4b0262c

Alioramus force-pushed the 2144 branch from b692011 to 4b0262c Compare February 26, 2017 18:33

zolotov requested changes Feb 28, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strings cannot be nil inspection #2871

Strings cannot be nil inspection #2871

Alioramus commented Feb 19, 2017

ignatov commented Feb 19, 2017

Alioramus commented Feb 19, 2017

zolotov left a comment

zolotov Feb 20, 2017

zolotov Feb 20, 2017

zolotov Feb 20, 2017

zolotov Feb 20, 2017 •

edited

Loading

zolotov Feb 20, 2017

zolotov Feb 20, 2017

zolotov Feb 20, 2017

zolotov Feb 20, 2017

zolotov Feb 20, 2017

zolotov Feb 20, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

Alioramus Feb 26, 2017 •

edited

Loading

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

zolotov Feb 26, 2017

Alioramus commented Feb 26, 2017

zolotov left a comment

zolotov Feb 28, 2017

zolotov Feb 28, 2017

zolotov Feb 28, 2017

zolotov Feb 28, 2017

zolotov commented Feb 28, 2017

zolotov commented Feb 28, 2017

		protected GoVisitor buildGoVisitor(@NotNull ProblemsHolder holder, @NotNull LocalInspectionToolSession session) {
		return new GoVisitor(){


		protected void check(GoTypeOwner var, GoExpression value){

		if(var == null \|\| value == null) return;

		}

		public static class GoChangeStringToDefaultValueQuickFix extends LocalQuickFixBase {

Strings cannot be nil inspection #2871

Are you sure you want to change the base?

Strings cannot be nil inspection #2871

Conversation

Alioramus commented Feb 19, 2017

ignatov commented Feb 19, 2017

Alioramus commented Feb 19, 2017

zolotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolotov Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alioramus Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alioramus commented Feb 26, 2017

zolotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zolotov commented Feb 28, 2017

zolotov commented Feb 28, 2017

zolotov Feb 20, 2017 •

edited

Loading

Alioramus Feb 26, 2017 •

edited

Loading