Rework GoStructInitializationInspection #2826

wbars · 2016-11-13T07:26:04Z

fixes #2819

zolotov · 2016-11-14T12:29:47Z

It's not really good idea to rename an inspection at least for two reasons:

I'd prefer to save vcs-history and git-blame data
The inspection is quite old and some users might have suppressed it with a comment //noinspection GoStructInitialization. The thing is that you changed the suppression id by renaming the class and old suppressions won't work anymore. The same thing for settings – some users could disable the inspection but since its ID was changed – the settings became invalid.

So please avoid inspections renaming until you have a really good reason

zolotov · 2016-11-14T12:30:06Z

@grenki please review the changes

grenki · 2016-11-17T16:00:13Z

src/com/goide/inspections/GoStructInitializationInspection.java

+
+  private static boolean areElementsNamesMatchesDefinitions(@NotNull List<String> elementsNames,
+                                                            @NotNull List<String> fieldDefinitionsNames) {
+    return range(0, elementsNames.size()).allMatch(i -> isNullOrEqual(elementsNames.get(i), getByIndex(fieldDefinitionsNames, i)));


reuse GoPsiImplUtil#getByIndex

Method is private and i don't see it as part of GoPsiImpl interface cause it's not related to psi operations.

I think we must have public util method like that. But while we haven't suitable util class it can be in GoPsiImplUtli

grenki · 2016-11-18T10:13:55Z

src/com/goide/inspections/GoStructInitializationInspection.java

+
+  private static void replaceElementsByNamed(@NotNull List<GoElement> elements,
+                                             @NotNull List<String> fieldDefinitionNames,
+                                             @NotNull Project project) {


Place project as first parameter, please

grenki · 2016-11-18T10:15:04Z

src/com/goide/inspections/GoStructInitializationInspection.java

-      if (startElement instanceof GoElement) {
-        startElement.replace(GoElementFactory.createLiteralValueElement(project, myStructField, startElement.getText()));
-      }
+      GoLiteralValue literal = ObjectUtils.tryCast(descriptor.getStartElement(), GoLiteralValue.class);


startElement can be null and invalid. You could check it

grenki · 2016-11-18T10:56:57Z

src/com/goide/inspections/GoStructInitializationInspection.java

+
+        List<String> elementsNames = getNames(literalValue.getElementList());
+        if (hasNull(elementsNames.toArray()) && areElementsNamesMatchesDefinitions(elementsNames, getFieldDefinitionsNames(structType))) {
+          holder.registerProblem(literalValue, "Unnamed field initializations", ProblemHighlightType.GENERIC_ERROR_OR_WARNING, QUICK_FIX);


I think better report problem for each element and it could be weak warning, because it can be valid code.

Actually, reporting the literal was one of the main point. Why do you think that report each element is better? We have to name all elements in literal to get valid code anyway, so what is the point in renaming one element.

And I believe that if we have mixed named and unnamed elements in the literal the code will not compile at all.

Reporting each element without keys

shows user only error element,

it doesn't mean that quickfix must rename only this element

big part of code which highlights as error looks bad :(

Anyway we mustn't highlight as error valid code like this: https://play.golang.org/p/4cP6PcSWCZ

And if you want to change problem highlighting type you could change level of inspection in gogland.xml

For me as user it would be a little strange, that invoke fix on element affect the parent, but if you fine with it, I don't mind, anyway my main desire was to invoke quickfix on literal.

grenki · 2016-11-18T12:06:12Z

src/com/goide/inspections/GoStructInitializationInspection.java

-        GoType refType = GoPsiImplUtil.getLiteralType(parent, false);
-        if (refType instanceof GoStructType) {
-          processStructType(holder, o, (GoStructType)refType);
+      public void visitCompositeLit(@NotNull GoCompositeLit compositeLit) {


And you could check literal value. If you look to go.bnf you can see that literal value not always has composite literal parent.

For example it's valid code

type S struct{int} _ = []S{ {1}, {2}, {3}} // parent of literal value {1} is GoValue {1}

wbars · 2016-11-22T05:07:54Z

src/com/goide/inspections/GoStructInitializationInspection.java

-    }
+  @Contract("_, null -> false; null, _ -> false")
+  private static boolean isSliceElementValue(@Nullable GoLiteralValue literalValue, @Nullable GoType parentLitType) {
+    return parentLitType instanceof GoArrayOrSliceType && literalValue != null && literalValue.getParent() instanceof GoValue;


For this operation (getting direct parent from nullable by class) I would like to move GoStructLiteralCompletion#parent to GoPsiTreeUtil since there are already plenty occurrences of this code in the plugin project, but I have some doubts.

There are already numerous methods for getting parent and, in particular, PsiTreeUtil#findFirstParent which, as I think, can become confusing with the new one. What do you think @grenki?

It's great idea! It very useful and convenient method.

PsiTreeUtil#findFirstParent and PsiTreeUtil#getParentOfType are hard to use in this case

wbars · 2016-11-29T10:39:20Z

Ping :)

zolotov · 2016-12-01T12:25:00Z

@wbars sorry for delay. Review is in progress. @grenki right?

grenki · 2016-12-01T12:57:45Z

src/com/goide/inspections/GoStructInitializationInspection.java

+    }
+  }
+
+  private static void replaceElementsByNamed(@NotNull Project project,


naming: better add keys to elements
Better to use names which are consistent with class names and its structure: we have GoNamedElement class and GoElement class hasn't name but it has GoKey and GoValue children

grenki · 2016-12-01T13:00:01Z

src/com/goide/inspections/GoStructInitializationInspection.java

+      if (value == null) continue;
+
+      GoElement namedElement = GoElementFactory.createLiteralValueElement(project, fieldDefinitionName, value.getText());
+      element.replace(namedElement);


You can add key and colon to element as children, but your way is also suitable

Yes, indeed. But I could not find a way to add colon and key to the element without several boilerplate lines of code so I would like to stay with the current solution.

grenki · 2016-12-01T16:55:02Z

src/com/goide/inspections/GoStructInitializationInspection.java

+        List<String> elementsNames = getNames(elements);
+        if (!areElementsNamesMatchesDefinitions(elementsNames, getFieldDefinitionsNames(structType))) return;
+
+        zip(elementsNames, elements).forEach(pair -> {


I think for loop more readable then this lambda

grenki · 2016-12-01T16:59:17Z

src/com/goide/inspections/GoStructInitializationInspection.java

+  private static GoStructType getStructType(@Nullable GoLiteralValue literalValue) {
+    GoCompositeLit lit = PsiTreeUtil.getParentOfType(literalValue, GoCompositeLit.class);
+    GoType litType = lit != null ? lit.getType() : null;
+    return isSliceElementValue(literalValue, litType)


it's wrong type calculating: you take composite lit type or type of array instead of literal value type. It can be a lot of nested literal values without composite literals

in this example intention inserts fields of B but literal type is S

type S struct { r, t int } type B struct{ S } var _ = []B{ S: {2, 3<caret>}}

Method GoPsiImplUtil.getLiteralType is so strange. But it can give you type of literal value. And you wouldn't need isSliceElementValue and getStructType methods

Example does not compile (should be S: S{2, 3<caret>} I guess), but quick fix on fixed version works. Maybe we should not highlight such cases at all? (when there is no composite literal for literal value). Anyway, someday there will be read code on these cases :)

Sorry for invalid example. I mean something like this

type S struct { r, t int } var _ = [][]S{ { {2, 2}, }, } var _ = map[string] []S{ "s": { {1, 2}, }, }

grenki · 2016-12-01T17:08:04Z

src/com/goide/inspections/GoStructInitializationInspection.java

+  }
+
+  @NotNull
+  private static List<String> getNames(@NotNull List<GoElement> elements) {


it isn't names :)

grenki · 2016-12-01T17:18:25Z

src/com/goide/inspections/GoStructInitializationInspection.java

-    }
+  @Contract("_, null -> false; null, _ -> false")
+  private static boolean isSliceElementValue(@Nullable GoLiteralValue literalValue, @Nullable GoType parentLitType) {
+    return parentLitType instanceof GoArrayOrSliceType && literalValue != null && literalValue.getParent() instanceof GoValue;


It's great idea! It very useful and convenient method.

PsiTreeUtil#findFirstParent and PsiTreeUtil#getParentOfType are hard to use in this case

grenki · 2016-12-01T17:20:49Z

src/com/goide/inspections/GoStructInitializationInspection.java

+      GoValue value = fieldDefinitionName != null && element.getKey() == null ? element.getValue() : null;
+      if (value == null) continue;
+
+      GoElement namedElement = GoElementFactory.createLiteralValueElement(project, fieldDefinitionName, value.getText());


you can inline namedElement
it is element with key, not named

wbars · 2016-12-08T10:53:53Z

PTAL

grenki · 2016-12-12T15:58:00Z

src/com/goide/inspections/GoStructInitializationInspection.java

+    return definition != null ? list(definition.getName()) : map(declaration.getFieldDefinitionList(), GoNamedElement::getName);
+  }
+
+  private static void registerProblemsForElementsWithoutKeys(@NotNull List<GoElement> elements,


better to make this method member of anonymous visitor

grenki · 2016-12-12T15:59:08Z

src/com/goide/inspections/GoStructInitializationInspection.java

   */
  @SuppressWarnings("WeakerAccess") public Boolean reportImportedStructs;

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


redundant line

grenki · 2016-12-12T16:17:15Z

src/com/goide/inspections/GoStructInitializationInspection.java

-  public JComponent createOptionsPanel() {
-    return new SingleCheckboxOptionsPanel("Report for local type definitions as well", this, "reportLocalStructs");
+  @Contract("null -> null")
+  private static GoStructType getLiteralStructType(@Nullable GoLiteralValue literalValue) {


this example is invalid

type S struct { r, t int } type B struct{ S } var _ = []B{ S: {2, 3}}

grenki · 2016-12-12T16:18:33Z

src/com/goide/inspections/GoStructInitializationInspection.java

-      processLiteralValue(holder, element, structType.getFieldDeclarationList());
-    }
+  @Nullable
+  private static String getFieldDefinitionName(@Nullable GoValue value) {


better to return named element in which key resolves:

PsiElement field = fieldName != null ? fieldName.resolve() : null; return field instanceof GoAnonymousFieldDefinition || field instanceof GoFieldDefinition ? ObjectUtils.tryCast(field, GoNamedElement.class) : null;

if you return field definition you wouldn't use getDefinition method in getFieldDefinitionType

grenki · 2016-12-12T16:31:27Z

tests/com/goide/inspections/GoStructInitializationInspectionTest.java

+  public void testInnerLiteralMap()                 { doQuickfixTest(); }
+  public void testInnerLiteralFieldWithKey()        { doQuickfixTest(); }
+  public void testMultipleInnerLiterals()           { doQuickfixTest(); }
+  public void testInnerLiteralWithoutType()           { doQuickfixTest(); }


grenki · 2016-12-12T16:40:47Z

testData/inspections/struct-initialization/literalValueWithoutCompositeLit.go

+}
+
+func main() {
+	var  _  = []B{ S: {<weak_warning descr="Unnamed field initializations">1<caret></weak_warning>}}


invalid test:

[]B { // S is invalid key because in array literals we use integer keys (indicies) S : { // expected literal type is B (parent literal has []B type) 1 // expected key is `S` (type is B) }, }

grenki · 2016-12-12T16:42:51Z

testData/inspections/struct-initialization/uninitializedStructImportedOnly.go

-		<weak_warning descr="Unnamed field initialization">"string"</weak_warning>,
-		<weak_warning descr="Unnamed field initialization">"string"</weak_warning>,
-		<weak_warning descr="Unnamed field initialization">nil</weak_warning>,
+		<weak_warning descr="Unnamed field initializations">"string"</weak_warning>,


initialization is better

grenki · 2016-12-12T16:46:39Z

src/com/goide/inspections/GoStructInitializationInspection.java

+  }
+
+  @NotNull
+  private static List<String> getKeys(@NotNull List<GoElement> elements) {


Better to use fields in which keys resolved instead names.

grenki · 2016-12-12T16:47:26Z

src/com/goide/inspections/GoStructInitializationInspection.java

+  }
+
+  @NotNull
+  private static List<String> getFieldDefinitionsNames(@Nullable GoStructType type) {


better return fields objects instead names

grenki · 2016-12-12T16:56:39Z

src/com/goide/inspections/GoStructInitializationInspection.java

-    return fieldDefinition != null ? fieldDefinition.getIdentifier().getText() : null;
+  private static GoType getUnderlyingType(@NotNull GoFieldDefinition fieldDefinition) {
+    GoType type = fieldDefinition.getGoType(null);
+    return type != null ? GoPsiImplUtil.getUnderlyingType(type) : null;


type.getUnderlyingType()

wbars · 2016-12-13T08:23:33Z

Force updated.

fixes go-lang-plugin-org#2819

grenki

Looks good, I think it can be merged after fixes last comments

Place new fixes in separate commit please

grenki · 2016-12-16T14:38:58Z

src/com/goide/inspections/GoStructInitializationInspection.java

+    GoNamedElement anonymousDefinition = ObjectUtils.tryCast(declaration.getAnonymousFieldDefinition(), GoNamedElement.class);
+    return anonymousDefinition != null
+           ? list(anonymousDefinition)
+           : map(declaration.getFieldDefinitionList(), definition -> ObjectUtils.tryCast(definition, GoNamedElement.class));


you can return List<? extends GonamedElement> and don't cast definittions

grenki · 2016-12-16T14:41:32Z

src/com/goide/inspections/GoStructInitializationInspection.java

+    GoFieldName fieldName = key != null ? key.getFieldName() : null;
+    PsiElement field = fieldName != null ? fieldName.resolve() : null;
+    return field instanceof GoAnonymousFieldDefinition || field instanceof GoFieldDefinition ? ObjectUtils
+      .tryCast(field, GoNamedElement.class) : null;


bad formatting

grenki · 2016-12-16T14:48:44Z

src/com/goide/inspections/GoStructInitializationInspection.java

+    GoKey key = PsiTreeUtil.getPrevSiblingOfType(value, GoKey.class);
+    GoFieldName fieldName = key != null ? key.getFieldName() : null;
+    PsiElement field = fieldName != null ? fieldName.resolve() : null;
+    return field instanceof GoAnonymousFieldDefinition || field instanceof GoFieldDefinition ? ObjectUtils


extract to util class and reuse GoMoveToStructInitializationIntention#isFieldDefinition

- Use generic return type in GoStructInitializationInspection#getFieldDefinitions - Move GoMoveToStructInitializationIntention#isFieldDefinition to GoPsiImplUtil

wbars · 2016-12-22T10:59:41Z

PTAL

wbars force-pushed the go-2819 branch 3 times, most recently from adbba13 to 2ad2b06 Compare November 14, 2016 04:32

zolotov added the pending update label Nov 14, 2016

zolotov self-assigned this Nov 14, 2016

wbars force-pushed the go-2819 branch 4 times, most recently from 1016569 to c8af522 Compare November 16, 2016 07:28

grenki reviewed Nov 18, 2016

View reviewed changes

wbars force-pushed the go-2819 branch 2 times, most recently from 0179518 to 925c529 Compare November 22, 2016 04:37

wbars commented Nov 22, 2016

View reviewed changes

zolotov added pending review and removed pending update labels Dec 1, 2016

grenki reviewed Dec 1, 2016

View reviewed changes

wbars force-pushed the go-2819 branch 5 times, most recently from 63d8b74 to 87177eb Compare December 8, 2016 10:53

wbars force-pushed the go-2819 branch from 87177eb to c0933e2 Compare December 9, 2016 04:47

grenki reviewed Dec 12, 2016

View reviewed changes

wbars force-pushed the go-2819 branch 2 times, most recently from 11bf872 to 0bec774 Compare December 13, 2016 08:20

Rework GoStructInitializationInspection

4ae828b

fixes go-lang-plugin-org#2819

wbars force-pushed the go-2819 branch from 0bec774 to 4ae828b Compare December 14, 2016 04:30

grenki reviewed Dec 16, 2016

View reviewed changes

Review marks

a5c00f3

- Use generic return type in GoStructInitializationInspection#getFieldDefinitions - Move GoMoveToStructInitializationIntention#isFieldDefinition to GoPsiImplUtil

Rework GoStructInitializationInspection #2826

Are you sure you want to change the base?

Rework GoStructInitializationInspection #2826

Conversation

wbars commented Nov 13, 2016

zolotov commented Nov 14, 2016

zolotov commented Nov 14, 2016

Choose a reason for hiding this comment

wbars Nov 18, 2016 • 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

wbars Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

grenki Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

wbars Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

grenki Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

wbars Nov 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbars commented Nov 29, 2016

zolotov commented Dec 1, 2016

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

wbars Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

grenki Dec 2, 2016 • 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

wbars commented Dec 8, 2016

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

wbars commented Dec 13, 2016

grenki left a comment • 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

wbars commented Dec 22, 2016

wbars Nov 18, 2016 •

edited

Loading

wbars Nov 18, 2016 •

edited

Loading

grenki Nov 18, 2016 •

edited

Loading

wbars Nov 18, 2016 •

edited

Loading

grenki Nov 18, 2016 •

edited

Loading

wbars Nov 22, 2016 •

edited

Loading

wbars Dec 2, 2016 •

edited

Loading

grenki Dec 2, 2016 •

edited

Loading

grenki left a comment •

edited

Loading