Fix function return value not used inspection #5338

MDoerner · 2020-01-04T21:39:19Z

This PR makes the FunctionReturnValueNotUsedInspection return the references where the function return value is discarded. It also reimplements the original inspection, now called FunctionReturnValueNeverUsedInspection, fixing the bugs I mentioned in issue #5337.

AddressOf expressions are now considered uses of the return value since the obtained pointer will typically be passed to a Win API function implemented outside of VBA. This function will probably use the return value when it calls the function via the pointer and it typically depends on the return type being correct.

Previously, it reported on declarations, which does not help in finding the offending call sites. An inspection with the old intended behaviour will be added again in the next commits.

This is basically what the old FunctionReturnValueNotUsedInspection was supposed to be.

codecov · 2020-01-06T19:37:13Z

Codecov Report

❗ No coverage uploaded for pull request base (next@2384e28). Click here to learn what that means.
The diff coverage is 92.59%.

@@           Coverage Diff           @@
##             next    #5338   +/-   ##
=======================================
  Coverage        ?   61.33%           
=======================================
  Files           ?     1146           
  Lines           ?    38812           
  Branches        ?     8236           
=======================================
  Hits            ?    23803           
  Misses          ?    13389           
  Partials        ?     1620

Impacted Files	Coverage Δ
...omplete/SelfClosingPairs/SelfClosingPairHandler.cs	`57.33% <100%> (ø)`
...ections/Concrete/ImplicitlyTypedConstInspection.cs	`100% <100%> (ø)`
...ar/PartialExtensions/VBAParserPartialExtensions.cs	`52.15% <87.5%> (ø)`

MDoerner · 2020-01-09T16:19:10Z

In the original PR description, I forgot to mention that this PR also makes an adjustment to the DeclarationFinder that allows to efficiently obtain the members of a module of a given declaration type, more precisely of those with a given flag.

It also introduces a base class for inspections investigating declarations of a given type.

retailcoder · 2020-01-12T21:11:12Z

Rubberduck.CodeAnalysis/Inspections/Concrete/FunctionReturnValueNeverUsedInspection.cs

+        //protected override IEnumerable<IInspectionResult> DoGetInspectionResults()
+        //{
+        //    var interfaceMembers = State.DeclarationFinder.FindAllInterfaceMembers().ToList();
+        //    var interfaceImplementationMembers = State.DeclarationFinder.FindAllInterfaceImplementingMembers();
+        //    var functions = State.DeclarationFinder
+        //        .UserDeclarations(DeclarationType.Function)
+        //        .Where(item => item.References.Any(r => !IsReturnStatement(item, r) && !r.IsAssignment))
+        //        .ToList();
+        //    var interfaceMemberIssues = GetInterfaceMemberIssues(interfaceMembers);
+        //    var nonInterfaceFunctions = functions.Except(interfaceMembers.Union(interfaceImplementationMembers));
+        //    var nonInterfaceIssues = GetNonInterfaceIssues(nonInterfaceFunctions);
+        //    return interfaceMemberIssues.Union(nonInterfaceIssues);
+        //}


Should this be removed?

Oh, forgot to remove that after testing that things still work. Will remove.

retailcoder · 2020-01-12T21:19:33Z

Rubberduck.CodeAnalysis/Inspections/Concrete/FunctionReturnValueNotUsedInspection.cs

@@ -46,159 +44,54 @@ namespace Rubberduck.Inspections.Concrete
    /// End Function
    /// ]]>
    /// </example>
-    public sealed class FunctionReturnValueNotUsedInspection : InspectionBase
+    public sealed class FunctionReturnValueNotUsedInspection : IdentifierReferenceInspectionBase


Keeping in mind that renaming an inspection is a breaking change as far as user configurations go, should this one be renamed DiscardedFunctionReturnValue since we're introducing FunctionReturnValueNeverUsed? That would do away with the "NotUsed"/"NeverUsed" dichotomy.

IIRC we have that dichotomy in other inspection names as well (didn't check though). If that is the case, then we should either "fix" all of them or keep all of them the same (to keep breaking change occurences minimal)

My problem with the old name is that it led to confusion more than once. It suggests that it is about the usage. However, the original inspection is about the declaration.

I just had a look: we only use NotUsed, so far. However, in all other cases, it is not ambiguous. (Here, the problem is that the return value can mean the individual value.)

I will rename FunctionReturnValueNotUsedInspection to FunctionReturnValueDiscardedInspection.

Should I rename FunctionReturnValueNeverUsedInspection to FunctionReturnValueAlwaysDiscardedInspection, as well?

Hmm. I like how "AlwaysDiscarded" avoids "Not"/"Never" wording. Might as well!

retailcoder · 2020-01-12T21:24:02Z

Rubberduck.Resources/Inspections/InspectionInfo.resx

@@ -217,7 +217,7 @@ If the parameter can be null, ignore this inspection result; passing a null valu
  <data name="SelfAssignedDeclarationInspection" xml:space="preserve">
    <value>An auto-instantiated object variable declaration at procedure scope changes how nulling the reference works, which can lead to unexpected behavior.</value>
  </data>
-  <data name="FunctionReturnValueNotUsedInspection" xml:space="preserve">
+  <data name="FunctionReturnValueNeverUsedInspection" xml:space="preserve">
    <value>A member is written as a function, but used as a procedure. Unless the function is recursive, consider converting the 'Function' into a 'Sub'. If the function is recursive, none of its external callers are using the returned value.</value>


IIUC the updated code handles recursive functions - the InspectionInfo text should be amended accordingly, no?

The inspection still does not report recursive functions; well, at least if the use their own return value. (The corresponding test still passes.)

Hm, reading that again, it suggests that recursive functions are reported. However, the tests test that it is not the case.

Should I change the description or actually implement the behaviour mentioned in the description, including appropriate tests?

Nah the inspection should just do what it needs to do, the description likely mentions recursiveness because inspection info strings sometimes mention shortcomings of inspections' implementations - if the shortcoming is fixed, the mention of it serves no purpose anymore. Let's reword the description.

Rubberduck.VBEEditor/Extensions/DictionaryExtensions.cs

…ubberduck-vba#5338

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

Also includes some German translations.

MDoerner added 3 commits January 4, 2020 16:26

Make FunctionReturnValueNotUsedInspection report references

7f414d1

Previously, it reported on declarations, which does not help in finding the offending call sites. An inspection with the old intended behaviour will be added again in the next commits.

Make DeclarationFinder store members by declarationType

9a51872

Reintroduce FunctionReturnValueNeverUsedInspection

761a2fd

This is basically what the old FunctionReturnValueNotUsedInspection was supposed to be.

MDoerner added the PR-Status: Review Requested No more commits, PR is ready for the eyes that need to see it. label Jan 4, 2020

Merge branch 'next' into FixFunctionReturnValueNotUsedInspection

14a021b

MDoerner mentioned this pull request Jan 12, 2020

Let reference inspections use base class #5347

Merged

retailcoder reviewed Jan 12, 2020

View reviewed changes

Rubberduck.VBEEditor/Extensions/DictionaryExtensions.cs Show resolved Hide resolved

retailcoder approved these changes Jan 12, 2020

View reviewed changes

retailcoder added the PR-Status: Conflicting PR can't be merged as it stands, conflicts must be resolved by the author. label Jan 13, 2020

MDoerner added 4 commits January 14, 2020 00:17

Make reporting of unhandled exceptions more useful

cc99a0b

Rename inspections and adjust resources according to comments to PR r…

2384e28

…ubberduck-vba#5338

Merge branch 'next' into FixFunctionReturnValueNotUsedInspection

66043bd

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

Alignment of punctuation at end of inspection resources

681fc3d

Also includes some German translations.

MDoerner removed the PR-Status: Conflicting PR can't be merged as it stands, conflicts must be resolved by the author. label Jan 15, 2020

retailcoder approved these changes Jan 17, 2020

View reviewed changes

retailcoder merged commit 9b3bd2d into rubberduck-vba:next Jan 17, 2020

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

MDoerner deleted the FixFunctionReturnValueNotUsedInspection branch February 19, 2020 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix function return value not used inspection #5338

Fix function return value not used inspection #5338

MDoerner commented Jan 4, 2020

codecov bot commented Jan 6, 2020 •

edited

Loading

MDoerner commented Jan 9, 2020

retailcoder Jan 12, 2020

MDoerner Jan 12, 2020

retailcoder Jan 12, 2020

Vogel612 Jan 12, 2020

MDoerner Jan 12, 2020

MDoerner Jan 13, 2020

retailcoder Jan 14, 2020

retailcoder Jan 12, 2020

MDoerner Jan 13, 2020 •

edited

Loading

MDoerner Jan 13, 2020

MDoerner Jan 13, 2020

retailcoder Jan 14, 2020

Fix function return value not used inspection #5338

Fix function return value not used inspection #5338

Conversation

MDoerner commented Jan 4, 2020

codecov bot commented Jan 6, 2020 • edited Loading

Codecov Report

MDoerner commented Jan 9, 2020

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 Jan 13, 2020 • 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

codecov bot commented Jan 6, 2020 •

edited

Loading

MDoerner Jan 13, 2020 •

edited

Loading