SetAssignmentWithIncompatibleObjectTypeInspection #5003

MDoerner · 2019-06-10T13:27:34Z

Closes #4981
Closes #5069
Closes #5071
Closes #3937

This PR already covers the scope layed out in the first issue above, i.e. it curently supports variable assignments on the RHS. However, I think that this is not too useful yet. So, I plan to extend the scope to new expressions, index expressions and member access expressions on the RHS.

Currently only supports variable assignments on the RHS. Also fixes a previously ignored test for the MoveCloserToUsageRefactoring.

# Conflicts: # Rubberduck.CodeAnalysis/Rubberduck.CodeAnalysis.xml

...rduck.CodeAnalysis/Inspections/Concrete/SetAssignmentWithIncompatibleObjectTypeInspection.cs

RubberduckTests/Inspections/SetAssignmentWithIncompatibleObjectTypeInspectionTests.cs

…ypeInspection into ISetTypeResolver implementation Currently only handles simple name expressions and instance expressions. Also fixed Declaration.FullAsTypeName for enums and UDTs.

# Conflicts: # Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs # Rubberduck.Resources/Inspections/InspectionInfo.de.resx # Rubberduck.Resources/Inspections/InspectionNames.Designer.cs # Rubberduck.Resources/Inspections/InspectionNames.de.resx # Rubberduck.Resources/Inspections/InspectionResults.Designer.cs # Rubberduck.Resources/Inspections/InspectionResults.de.resx

…-object expressions null is only returned in case we cannot resolve the Set type, primarily because of member accesses on objects of type Variant and Object.

This includes a workaround requires because built-in types are currently resolved as lExpressions, more precisely as simpleNameExpressions.

Previously, everything matched by builtInTypeExpr was already consumed by lExpr. Since all built-in types are keywords, this should not cause problems in most cases. The only exception would be foreign identifiers whose name is a built-in type. (This should be very rare and is problematic anyway.) This change required patching up the resolver as well to deal with builtInTypes.

…k in (with) dictionary access expressions

…th compatible parameter list with only optional arguments

…resolver

# Conflicts: # Rubberduck.Resources/Inspections/InspectionInfo.Designer.cs # Rubberduck.Resources/Inspections/InspectionInfo.resx # Rubberduck.Resources/Inspections/InspectionNames.Designer.cs # Rubberduck.Resources/Inspections/InspectionNames.resx # Rubberduck.Resources/Inspections/InspectionResults.Designer.cs # Rubberduck.Resources/Inspections/InspectionResults.resx

Also cleans up NonReturningFunctionInspection and makes Selection honor the contract of IComparable<Selection>.

MDoerner · 2019-08-10T12:26:39Z

This PR is largely finished now. It introduces a SetTypeResolver able to provide the set type name and declaration for all expressions for which these can be reolved at compile time. This is used to supply the set type name for the inspection in order to compare it with the LHS of the Set assignment.

The current implementation uses identifier references to determine the Set type. This has the advandage over putting the information on the contextx themselves that no further cash invalidation is required.

I still plan to do some clean up in IndexDefaultBinding and BoundExpressionVisitor around the resolution of index expressions. However, I would already apprechieate any feedback I could get, especially on the Set resolver.

These reference the entire index expression and have the new flag IsArrayAccess set to true. To resolve everything correctly, the entire IndexDefaultBinding got an overhaul. Also replaces the tests of ImplicitDefaultMemberAssignmentInspection since they previously only passed because the resolution of the assignment target was wrong.

…to the inspection itself This also makes the ImplicitDefaultMemberAssignmentInspection work correctly without tweaks to the resolver..

Previously the options without explicit parentheses were preferred and matched to expressions with parentheses consuming them as part or the argument.

MDoerner · 2019-08-13T23:12:02Z

I basically rewrote the resolution of index expressions in the last commits to simplify things in the resolver. This includes attaching references to array accesses with a new flag IsArrayAccess. I also added the new flag IsDefaultMemberAccess to better identify these accesses. These are now also part of the IndexExpression, which was the basic change alllowing for a cleaner handling of these situations.

I do not plan to add anything further to this PR excpt in response to review comments.

bclothier

Excellent job, MDoerner! Mainly nitpicks. I also have one general comment - there are a numbers of TODOs but the comment there does not really explain what exactly needs to be done. For example, instead of Need to improve checks, it should explain what is missing so that maintainers who happen to go through will understand what needs doing there.

Rubberduck.CodeAnalysis/Inspections/Concrete/AssignedByValParameterInspection.cs

Rubberduck.Parsing/Binding/Bindings/DictionaryAccessDefaultBinding.cs

Rubberduck.Parsing/Binding/Bindings/IndexDefaultBinding.cs

Rubberduck.Parsing/Grammar/VBAParser.g4

Rubberduck.Parsing/TypeResolvers/SetTypeResolver.cs

Rubberduck.VBEEditor/Selection.cs

RubberduckTests/ExpressionResolving/SetTypeResolverTests.cs

RubberduckTests/Grammar/ResolverTests.cs

RubberduckTests/Refactoring/MoveCloserToUsageTests.cs

…ess expressions

…Error

RubberduckTests/Inspections/ImplicitActiveWorkbookReferenceInspectionTests.cs

bclothier · 2019-08-14T23:27:52Z

RubberduckTests/Inspections/ObjectVariableNotSetInspectionTests.cs

        [Test]
-        [Ignore("Broken by COM collector fix. See comment on test.")]
+        //[Ignore("Broken by COM collector fix. See comment on test.")]


If it's working, do we really need the comment (both of them) anymore?

Oh, I missed to remove the ignore line entirely. Will do that later.

retailcoder

Very clean code as always - only a few minor suggestions, other than that LGTM.

retailcoder · 2019-08-15T04:59:49Z

...rduck.CodeAnalysis/Inspections/Concrete/SetAssignmentWithIncompatibleObjectTypeInspection.cs

+        /// <why>
+        /// The VBA compiler does not check whether different object types are compatible. Instead there is a runtime error whenever the types are incompatible.
+        /// </why>
+        /// <example>


Suggested change

/// <example>

/// <example hasResult="true">

retailcoder · 2019-08-15T05:00:29Z

...rduck.CodeAnalysis/Inspections/Concrete/SetAssignmentWithIncompatibleObjectTypeInspection.cs

+        /// End Sub
+        /// ]]>
+        /// </example>
+        /// <example>


Suggested change

/// <example>

/// <example hasResult="false">

retailcoder · 2019-08-15T05:03:16Z

Rubberduck.Parsing/Binding/Bindings/DictionaryAccessDefaultBinding.cs

+                */
+
+                return ResolveRecursiveDefaultMember(defaultMember, defaultMemberClassification, argumentList, expression, recursionDepth);
+            }


Great job with the comments there 👍

They are basically copied from IndexDefaultBinding, where autoboosh added them. I just modified them to refer to a dictionary access expression instead of an index expression.

retailcoder · 2019-08-15T05:06:41Z

Rubberduck.Parsing/Symbols/IdentifierReference.cs

@@ -60,6 +64,10 @@ public class IdentifierReference : IEquatable<IdentifierReference>

        public bool IsSetAssignment { get; }

+        public bool IsDefaultMemberAccess { get; }


Brilliant 👍

retailcoder · 2019-08-15T05:10:30Z

Rubberduck.Parsing/TypeResolvers/SetTypeResolver.cs

+        //Please note that the lExpression is the (whitespace) index expression itself and not the lExpression it contains. 
+        private Declaration ResolveIndexExpressionAsArrayAccess(VBAParser.LExpressionContext actualIndexExpr, QualifiedModuleName containingModule, DeclarationFinder finder)
+        {
+            // A n array access references the entire (whitespace)indexExpr.


Suggested change

// A n array access references the entire (whitespace)indexExpr.

// An array access references the entire (whitespace)indexExpr.

retailcoder · 2019-08-15T05:13:08Z

Rubberduck.Resources/Inspections/InspectionNames.resx

@@ -386,6 +386,9 @@
  <data name="ObsoleteWhileWendStatementInspection" xml:space="preserve">
    <value>Use of obsolete 'While...Wend' statement</value>
  </data>
+  <data name="SetAssignmentWithIncompatibleObjectTypeInspection" xml:space="preserve">
+    <value>Set Assignment With Incompatible Object Type</value>


Suggested change

<value>Set Assignment With Incompatible Object Type</value>

<value>Set assignment with incompatible object type</value>

retailcoder · 2019-08-15T05:15:43Z

RubberduckTests/Inspections/AssignedByValParameterInspectionTests.cs

+Attribute Foo.VB_UserMemId = 0
+End Property
+Public Property Let Something(ByVal value As Long)
+Attribute Foo.VB_UserMemId = 0


Not that it makes a difference, but is this legal code? IIUC only one member is allowed to have VB_UserMemId = 0... I've always only ever put it on the Property Get member.

I have no idea. I guess the VBE would just ignore one of them.

RubberduckTests/Inspections/ImplicitActiveWorkbookReferenceInspectionTests.cs

…tTypeInspection Also changes the way the set assignments are obtained to increase performance when libraries are referenced.

MDoerner added 2 commits June 10, 2019 15:13

Add initial SetAssignmentWithIncompatibleObjectTypeInspection

a993dfd

Currently only supports variable assignments on the RHS. Also fixes a previously ignored test for the MoveCloserToUsageRefactoring.

Merge branch 'next' into IncompatibleObjectTypeInspection

de99bcf

# Conflicts: # Rubberduck.CodeAnalysis/Rubberduck.CodeAnalysis.xml

MDoerner added the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Jun 10, 2019

bclothier reviewed Jun 11, 2019

View reviewed changes

...rduck.CodeAnalysis/Inspections/Concrete/SetAssignmentWithIncompatibleObjectTypeInspection.cs Show resolved Hide resolved

RubberduckTests/Inspections/SetAssignmentWithIncompatibleObjectTypeInspectionTests.cs Show resolved Hide resolved

MDoerner added 13 commits August 4, 2019 19:04

Extract set type resolution from SetAssignmentWithIncompatibleObjectT…

e369382

…ypeInspection into ISetTypeResolver implementation Currently only handles simple name expressions and instance expressions. Also fixed Declaration.FullAsTypeName for enums and UDTs.

Add support for (with) member access expressions to SetTypeResolver

47076a6

Explicitly return NotAnObject from SetTypeReolver.SetTypeName for non…

242cb42

…-object expressions null is only returned in case we cannot resolve the Set type, primarily because of member accesses on objects of type Variant and Object.

Add support for all non lExpressions to SetTypeResolver

fd0a701

This includes a workaround requires because built-in types are currently resolved as lExpressions, more precisely as simpleNameExpressions.

Attach references to the called default member to the exclamation mar…

ae99ed2

…k in (with) dictionary access expressions

Fix rubberduck-vba#5069

d9e6679

Fix resolution of dictionary access expression for default members wi…

57fd753

…th compatible parameter list with only optional arguments

Add support for (with) dictionary access expressions to the Set type …

44d53c2

…resolver

Add IsDefaultMemberAccess to IdentifierReference

8148307

Also cleans up NonReturningFunctionInspection and makes Selection honor the contract of IComparable<Selection>.

Add support for index expressions to SetTypeResolver

d46f190

MDoerner added PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. and removed PR-Status: WIP Pull request is work-in-progress, more commits should be expected. labels Aug 10, 2019

MDoerner marked this pull request as ready for review August 10, 2019 12:26

bclothier mentioned this pull request Aug 12, 2019

Code inspection give incorrect Set warnings for variants and indexed collections #4161

Closed

MDoerner added 3 commits August 13, 2019 22:55

Move band-aid for AssignmentToByValParameterInspection from resolver …

6348fff

…to the inspection itself This also makes the ImplicitDefaultMemberAssignmentInspection work correctly without tweaks to the resolver..

Reorder alternatives in annotationArgList in grammar

39402e8

Previously the options without explicit parentheses were preferred and matched to expressions with parentheses consuming them as part or the argument.

bclothier reviewed Aug 14, 2019

View reviewed changes

MDoerner added 3 commits August 14, 2019 22:10

Fix resolution of default member and array accesses on dictionary acc…

02cad44

…ess expressions

Clarify todos and add comments

c5ab99e

Fix named argument resolution on default member accesses

426dc00

MDoerner added 2 commits August 15, 2019 00:47

Set Severity of SetAssignmentWithIncompatibleObjectTypeInspection to …

7a7f2e7

…Error

Unignore tests passing now

8668b13

bclothier reviewed Aug 14, 2019

View reviewed changes

RubberduckTests/Inspections/ImplicitActiveWorkbookReferenceInspectionTests.cs Show resolved Hide resolved

bclothier reviewed Aug 14, 2019

View reviewed changes

retailcoder approved these changes Aug 15, 2019

View reviewed changes

MDoerner added 3 commits August 15, 2019 17:53

Merge branch 'next' into IncompatibleObjectTypeInspection

56165d1

Fix comments

2fe9ab9

Remove restriction to variables on SetAssignmentWithIncompatibleObjec…

0ba0c90

…tTypeInspection Also changes the way the set assignments are obtained to increase performance when libraries are referenced.

bclothier approved these changes Aug 15, 2019

View reviewed changes

bclothier merged commit 10a0572 into rubberduck-vba:next Aug 15, 2019

MDoerner removed the PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. label Aug 29, 2019

MDoerner mentioned this pull request Aug 29, 2019

Inspection for incompatible function/getter return value #3603

Closed

bclothier mentioned this pull request Sep 2, 2019

Resolver needs to support resolving bracketed expressions into member calls. #2565

Open

MDoerner deleted the IncompatibleObjectTypeInspection branch February 19, 2020 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SetAssignmentWithIncompatibleObjectTypeInspection #5003

SetAssignmentWithIncompatibleObjectTypeInspection #5003

MDoerner commented Jun 10, 2019 •

edited

MDoerner commented Aug 10, 2019 •

edited

MDoerner commented Aug 13, 2019

bclothier left a comment

bclothier Aug 14, 2019

MDoerner Aug 15, 2019 •

edited

retailcoder left a comment

retailcoder Aug 15, 2019

retailcoder Aug 15, 2019

retailcoder Aug 15, 2019

MDoerner Aug 15, 2019

retailcoder Aug 15, 2019

retailcoder Aug 15, 2019

retailcoder Aug 15, 2019

retailcoder Aug 15, 2019

MDoerner Aug 15, 2019

		@@ -60,6 +64,10 @@ public class IdentifierReference : IEquatable<IdentifierReference>

		public bool IsSetAssignment { get; }

		public bool IsDefaultMemberAccess { get; }

	// A n array access references the entire (whitespace)indexExpr.
	// An array access references the entire (whitespace)indexExpr.

	<value>Set Assignment With Incompatible Object Type</value>
	<value>Set assignment with incompatible object type</value>

SetAssignmentWithIncompatibleObjectTypeInspection #5003

SetAssignmentWithIncompatibleObjectTypeInspection #5003

Conversation

MDoerner commented Jun 10, 2019 • edited

MDoerner commented Aug 10, 2019 • edited

MDoerner commented Aug 13, 2019

bclothier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDoerner Aug 15, 2019 • edited

Choose a reason for hiding this comment

retailcoder 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

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

MDoerner commented Jun 10, 2019 •

edited

MDoerner commented Aug 10, 2019 •

edited

MDoerner Aug 15, 2019 •

edited