Parameters required

[Voorhout]

At Rubberduck's advice, I removed parameters (like 'Cancel' of 'PrintCount') from event procedures as they were deemed unnecessary. Now, however, I get a large number of Compile errors saying the procedure declaration does not match description of event or procedure having the same name. I therefore think your advice was wrong. Maybe you should address this issue.

[Vogel612]

Could you show us the "before" procedure as well as how it's called? The more information you give us, the better we can help resolve this.

[Voorhout]

Dear Clemens,

For instance, I have a form with a list box lboTyp, and I want to do something when the user leaves that box. I therefore created an event 'On Exit' from the object's properties box:

Public Sub lboTyp_Exit(Cancel As Integer) lboTyp.SpecialEffect = flat End Sub

This worked fine. Rubberduck, however, on Code Inspection under Code Quality Issues, reported that the parameter 'Cancel' was never used or referred to, and that I should consider removing it. I followed that advice, but then got a compile error because the 'Procedure declaration does not match the description of event or procedure having the same name'. I therefore think Rubberduck's was wrong in advising the removal of the parameter.

I hope this clarifies the issue.

Kind regards, Joost van Asten

On Tue, 7 May 2019 at 13:32, Clemens Lieb notifications@github.com wrote:

Could you show us the "before" procedure as well as how it's called? The more information you give us, the better we can help resolve this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rubberduck-vba/Rubberduck/issues/4949#issuecomment-490041721, or mute the thread https://github.com/notifications/unsubscribe-auth/AMAIBRQVTMW73KJEJWQX5HTPUFSFXANCNFSM4HLH3Q2Q .

[retailcoder]

@Vogel612 it's an event handler and we're not resolving it as such because MSForms interfaces are a mess. #4570 is a similar issue with the same underlying cause.

@voorhout this is a known issue. Event handler procedures' signatures must always match exactly with their respective event's definition. You can't add, remove, or reorder event handler paramerers, or change their type or how they're passed (ByRef/ByVal), and Rubberduck knows this very well, as it knows that no event handler should have direct callers. But UserForm/control event procedures, we're not able to see that they are event handlers because the form and its controls implement several interfaces that each expose several events - this is a very complex problem.

Rubberduck inspections can also not see that macro procedures attached to a worksheet shape or button, are invoked by that shape/button - or a UDF invoked by some cell's formula - these procedures will be reported as "not used" - because we are indeed not seeing them used anywhere in the code.

Whether you use an open-source, free software thar does static code analysis, or a AAA-priced IDE add-in to do the same, the base principle is the same: they are tools that can definitely help you in certain situations, even find & fix some bugs, and unless they understand the code exactly as VBA does, there will be false positives and false negatives, and your own judgement should always prevail.

Removing an unused parameter or procedure isn't the only available fix: "ignore once" is always there too, waiting for your better judgement to step in and tell Rubberduck "thanks, I got this" with an '@Ignore ParameterNotUsed annotation.

You can also ignore an inspection's results across an entire module, by annotating the module with '@IgnoreModule ParameterNotUsed.

[retailcoder]

I hope that didn't come off as "well duh, use your judgement!" - that was absolutely not the intent. What I meant was, you're always in charge, Rubberduck is never going to force you to break your code, and if you see it making recommendations that would, then you definitely should ignore them: your understanding of the code (and/or your coding preferences) should always take precedence.

That said the particular case of MSForms interfaces is indeed very annoying. IIRC we see everything through its MSForms.Control interface, so type-specific properties will be reported as a "member not on interface", and type-specific event handlers will be reported as "not used", as long as we don't have a way to discover the runtime type of MSForms controls.

I think what we can do, is determine that we're in a form module, and issue a slightly different inspection result message to mention the highly possible false positive. That wouldn't cover all bases, but would be better than nothing.

[retailcoder]

Actually, we could simply add a note to the existing inspection info string, like "Ignore if this parameter is in the scope of an event handler procedure: Rubberduck might not be resolving that event handler as such - watch for other false positives". Another fix could be added too, e.g. "Assign ByRef parameter value", and Rubberduck could add e.g. Cancel = 0 at the top of the procedure. I don't like that because it's a redundant assignment to the type's default value... But it would eliminate the false positive just as well.

[Vogel612]

The change we should do here is to make the RemoveParameter refactoring smarter. It should recognize when it's changing an Event Handler and abort the refactoring if the Event is not userdefined.
Since the Refactoring is called by the quickfix, that should also result in the Quickfix not completing. Of course we would need to notify the user why the refactoring has been aborted.

If the Event is user-defined and we're not refactoring the Event declaration, we should also pop a warning, and refactor the Event Declaration (as well as all handlers) to avoid a compilation issue caused by Rubberduck.
This could even go so far as to check whether the other handlers also do not use the parameter and aborting the refactoring if that is not the case (something like a safe delete).

[MDoerner]

Some comments on the last suggestion: part of this is actually implemented, just not fully. Moreover, I would like to differ on the treatment of user-defined events. These should be treated the same way as interfaces, i.e. the event should be modified instead after user confirmation. (We do it this way for other refactorings.)

Currently, the switch to a user-defined event is performed without confirmation.

The confirmation request belongs into the presenter, like the one for interfaces.

Regarding built-in events, we should probably add a check for this situation and throw a TargetIsNotUserDefinedException. More precisely, we should not only search for user-defined events, as implemented right now, but all, and then throw if we find a built-in one. The notifier for this refactoring will need to be adapted as well in order to provide a useful message.

Note that there is no confirmation for the entry points into refactorings designed for quickfixes. It is the job of the inspection to only provide valid results.

[retailcoder]

Confirming as fixed.