New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed guidelines for COM visible RD features #2903

Open
comintern opened this Issue Mar 18, 2017 · 17 comments

Comments

Projects
None yet
7 participants
@comintern
Contributor

comintern commented Mar 18, 2017

Extending the public Unit Testing interfaces sparked this discussion in chat, and I thought it would be a good idea to propose some developer guidelines for declaring members on RD's COM visible interfaces. Tagging this as discussion, because this is basically RD's "public face" in the realm of COM interop, but more importantly some of the proposals would require potentially breaking changes to our public APIs.

Proposal 1:
My next PR is going to consolidate all of the assigned public GUIDs into a static class RubberduckGuid and all of the publicly declared ProgIds into a static class RubberduckProgId. I intend to do the same thing for Rubberduck.SourceControl. This will make it a lot easier to keep track of what we're exposing externally. So... all publicly assigned GUIDs and ProgIds should be assigned in these classes. The annotations should look similar to this:

    [Guid(RubberduckGuid.AssertClassGuid)]
    [ProgId(RubberduckProgId.AssertClassProgId)]

Proposal 2:
All ComVisible classes should have the default interface explicitly declared and also made ComVisible. This prevents the build from implicitly generating an interface to assign it to. The class should also specifically declare that interface with [ComDefaultInterface(typeof(IFoo))].

Proposal 3:
Only classes that we intend the user to new up should be ComVisible - all other functionality should only be extended with a public interface. Any procedures or properties that are not on the exposed ComVisible interface should be declared as internal, protected or private.

Proposal 4:
All ComVisible interfaces, classes, and enums should be explicitly assigned a GUID to prevent the build from generating and assigning one.

Proposal 5 (potentially breaking):
All assigned GUIDs should be in the same "GUID-space". If you take a look at the type libraries for the Office components, you'll notice that they all use a "base GUID" of...

########-0000-0000-C000-000000000046

...and assign members sequentially. This simplifies finding them in the registry.

Proposal 6 (potentially breaking):
Public enums should be declared with an Rd prefix, and public enum members and constants should be declared with an rd prefix. This follows the VBA and Office convention, but more importantly it avoids naming collisions for members that have global scope in VBA. If the enums or constants are also used internally, they should be aliased to follow the .NET convention. For example:

    [ComVisible(true)]
    public enum RdDeclarationType
    {
        rdProject = DeclarationType.Project,
        rdStandardModule = DeclarationType.ProceduralModule,
        rdClassModule = DeclarationType.ClassModule,
        // ...
    }

Proposal 7:
All public members should follow VBA naming conventions, not C# or internal naming conventions. This means that ComVisible functions and parameters should be PascalCase instead of camel case. This avoids the "only one case" rule that the VBE enforces and makes our functionality look more "VBA-like" in the IntelliSense tool-tips. I have a hard enough time avoiding the identifier Value in my VBA code that I don't want a reference to RD changing every single Cells(r, c).Value all to Cells(r, c).value. The internal implementations of interfaces should use C# casing unless the class itself is ComVisible Example:

// in interface IVerify (ComVisible):
void Parameter(string Parameter, object Value, int Invocation = 1, string Message = "");
// in class Verifier : IVerify (not ComVisible)
public void Parameter(string parameter, object value, int invocation = 1, string message = "")

Proposal 8:
Unless it is completely unavoidable, ComVisible interfaces should not implement IDisposable or inherit from interfaces that implement it. This one should be a no-brainer.

Proposal 9:
Structs should not be ComVisible. If you need to expose one for some reason, wrap in with a class and an interface.

@comintern

This comment has been minimized.

Contributor

comintern commented Mar 19, 2017

Proposal 10:
The RD COM interfaces need to be designed specifically with "VBA friendliness" in mind. That means that no ComVisible interface should return a type that that isn't natively supported in VBA (i.e. any unsigned types other than Byte). For a general reference of types that can be used by VBA, see the VarEnum lookup in the ComVariant class. If in doubt, cast to a type that you know is supported. Also, exposing IntPtrs should be avoided like the plague.

@rubberduck203

This comment has been minimized.

Member

rubberduck203 commented Mar 19, 2017

I'd like to hear more of the why behind #9. Is there an issue with exposing structs?

Also, thank you for bringing this up. It's an important conversation to have & I'd love to see the results moved into the wiki.

@comintern

This comment has been minimized.

Contributor

comintern commented Mar 19, 2017

@rubberduck203 - Marshalling. IIR the build process auto-wraps them with an interface (I could be mistaken about that) because they can't be marshalled cleanly into a Variant. The root of it is similar to the reason that VB throws compile errors if you try to expose a public Type in a class module. When structures get coerced into a Variant, they wind up as a VT_USERDEFINED or a pointer. There isn't a way for VB to late bind to them because the information as to the memory layout isn't built into the Variant. That said, it's fairly easy and probably more consumer-friendly to just wrap them.

@rubberduck203

This comment has been minimized.

Member

rubberduck203 commented Mar 19, 2017

To go along with Proposal 2, all members of Com visible interfaces should explicitly declare their [DispId()] like was done here.

It gives us the freedom of rearranging the members in the C# code without breaking the COM interface. Without it, reordering the members is a breaking change.

@rubberduck203

This comment has been minimized.

Member

rubberduck203 commented Mar 19, 2017

5, 6, & 7 are all breaking changes, but they're good changes to be making. I need to get some sleep, but let's talk about how these could be done gracefully. Let's make these standards for any new development and figure out how to make the transition.

@ThunderFrame

This comment has been minimized.

Member

ThunderFrame commented Mar 19, 2017

Re 5: CAFED0CC or D0CCBABE or BAB1D0C1, or BA51CD0C?

@comintern

This comment has been minimized.

Contributor

comintern commented Mar 19, 2017

Completely agree with @rubberduck203's observation that [DispId()] should be specified for all ComVisible members. In addition, and mainly to document from chat, DispId(0) should not be used. In general, RD should be discouraging the implicit use of default members in user VBA code. Offering a default member on our interfaces does little to discourage the practice.

@pgeerkens

This comment has been minimized.

pgeerkens commented Sep 2, 2017

@comintern @rubberduck203 These mostly sound really good, and I am in the process of implementing them in Ribbon Dispatcher. However I am unsure about strict adherence to #3). The Ribbon Dispatcher aims to generate events in DOT NET and expose those to VBA clients. The only way I have seen to do that is to make the classes ComVisible with a [ClassInterface(ClassInterfaceType.None)] attribute and an appropriate [ComSourceInterfaces(typeof(IClickedEvents))] specification. Or have I missed an alternative way to implement this requirement?

@rubberduck203

This comment has been minimized.

Member

rubberduck203 commented Sep 3, 2017

@pgeerkens I've honestly never exposed events from .Net to COM before, so it's possible we missed something in our proposals here. Do you mind taking a stab at rewriting Prop 3 to include a standard for events?

@ThunderFrame

This comment has been minimized.

Member

ThunderFrame commented Sep 3, 2017

The standard way that most MS events are exposed, for example, are as an IWorksheet for the members, and an IWorksheetEvents for the events, all wrapped up as a Worksheet CoClass. I suggest we do the same.

@pgeerkens

This comment has been minimized.

pgeerkens commented Sep 3, 2017

@ThunderFrame : So far I have allowed the compiler to do the heavy lifting on this for me - Are there enough benefits to be had by wiring this myself instead? That is not clear to me. I also was hoping to not have to dive that deeply into the plumbing details.

@comintern : Perhaps an explicit exception on the "No Default Members" guideline might be made for members such as Item and Field that are equivalent to a C# "indexer". I am sure we have all found those immensely convenient over the past 10 years.

@retailcoder

This comment has been minimized.

Member

retailcoder commented Sep 3, 2017

@pgeerkens following the VBA tag on Stack Overflow shows that implicit calls to default members cause dozens of bugs to be written by dozens of VBA devs on the daily - personally I'm fine with Item only if the class is clearly a collection type with a pluralized name, and if it's iteratable in a For Each loop. If it can't be iterated and the class isn't clearly a collection, exposing a default member is a no-no. There is no need for any default member beyond that (i.e. no Field).

@pgeerkens

This comment has been minimized.

pgeerkens commented Sep 3, 2017

@retailcoder : That works for me. I guess my real point is that since so many VBA programmers are really Financial Analysts at heart, we may have more beneficial effect on practice by nudging the culture rather than attacking it. Most of my current team(at work) regards the mere use of Class Modules as some sort of black magic.

@rubberduck203

This comment has been minimized.

Member

rubberduck203 commented Sep 3, 2017

And you've just summed up my entire career in software development @pgeerkens.

so many VBA programmers are really Financial Analysts at heart

@pgeerkens

This comment has been minimized.

pgeerkens commented Sep 3, 2017

re Prop. #5: I wasn't entirely sure about the purpose of the UnitTestingGuidspace = "-43F0-3B33-B105-9B8188A6F040"; assignment, so I added RibbonDispatcherGuidspace = "-43F1-3B33-B105-9B8188A6F040"; for the Ribbon Dispatcher (and started from a base of ...9400). However there is more than enough space in 9400..94D9 for the Ribbon Dispatcher interfaces and classes. Can anyone weigh in on the intent of the UnitTestingGuidspace, and whether Ribbon Dispatcher should squeeze underneath it or jump on top as currently in place?

@Vogel612

This comment has been minimized.

Member

Vogel612 commented Nov 26, 2017

Is this in any way enforced? Is there a consensus?

@bclothier

This comment has been minimized.

Contributor

bclothier commented May 1, 2018

With the analyzer introduced in #3975, we are now able to enforce some of the guidelines; proposal 1 to 5 are now effectively enforced. A separate PR may be needed to enforce the rest of proposals, since they deal with a different type of problems so they need their separate analyzer.

It is suggested that we enforce this via analyzers, rather than writing a wiki article.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment