False positive inspection results are generated for Public fields in Class Modules that define an interface

[BZngr]

Version 2.5.2.x (debug) OS: Microsoft Windows NT 10.0.19042.0, x64 Host Product: Microsoft Office x64 Host Version: 16.0.14527.20276 Host Executable: EXCEL.EXE

Public fields are acceptable declarations in classes that define interfaces. ImplementInterfaceRefactoring correctly implements a Let/Set/Get Property for each Public field in the interface definition class. These Public fields are flagged by the VariableNotUsed and VariableNotAssigned inspections.

An interface defined like:

Option Explicit

'@Exposed
'@Interface

Public TestNumber As Long

Public Sub RunTests()
End Sub

Will result in Inspection results for TestNumber

Expected behavior:

Class Modules that have an '@Interface annotation or classes that are used as part of an Implements statement should not be flagged by the VariableNotUsed or VariableNotAssigned inspections.

[BZngr]

Related #5291

[MDoerner]

@retailcoder @Vogel612 Can I get your opinion here? Should we report unused Public members of an interface or not? @BZngr leans towards not reporting them and I would rather lean towards reporting them all, at least unless they are in an exported class module.

In any way, this needs some cleanup because the handling is currently inconsistent. We do not report unused public functions and procedures in interface modules, but we report unused public variables.

[retailcoder]

In any way, this needs some cleanup because the handling is currently inconsistent. We do not report unused public functions and procedures in interface modules, but we report unused public variables.

Agreed, this needs a bit more consistency. I'm not sure the rationale for ignoring public procedures in interfaces is completely sound, it looks like something of a historical baggage around properly resolving abstract member usage; since an abstract field is always implemented as a property, consistency demands that we also ignore such abstract fields, and thus treat a public variable as we would a public member, in the context of "variable not used" against an abstract class/interface... But that doesn't seem right.

I think that, given a class that's implemented by another class, or marked with an @Interface annotation, we should consider as "not used" any member (field or procedure) that has no direct references, regardless of whether or not the abstraction is actually implemented. The idea is that we expect code to be written against that interface, so we should flag the abstract members that nobody is calling.

The corollary is that we should be ignoring implementations of these abstract members, because we are expecting the concrete implementations to not be invoked directly.

[MDoerner]

We already ignore all results for the implementing members.

I would vote to still not flag any public members of fields in an interface module that has the Exposed attribute. These are kind of clearly intended for external use by another project.

[Vogel612]

I have nothing to add to the discussion itself. My vote is to keep results for public members that are not exposed and treat an exposed module like an @EntryPoint for the purposes of all UnusedX inspections

[MDoerner]

OK, then I will align the handling in this direction.

BTW, this is actually consistent with how major tools for other languages do this, e.g. visual studio and intellij.