Fix type name resolution and implement short form properties in COM collector. #4189

comintern · 2018-07-16T00:40:17Z

Closes #3326
Partially addresses #4096

IMPORTANT: This PR does not update the serialized declarations used by the unit tests. At some point these apparently became stale, so 34 tests are failing with the new declarations. I will PR the new declarations when #4182 is resolved.

@future: If you make any changes that effect the behavior of the COM collection, you HAVE TO update the serialized declarations files for the unit tests. If you don't, it basically invalidates the tests and potentially masks bugs.

…n changes.

…ollection changes." This reverts commit 290e7e2.

codecov · 2018-07-16T00:54:20Z

Codecov Report

Merging #4189 into next will decrease coverage by 0.23%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##             next    #4189      +/-   ##
==========================================
- Coverage   52.46%   52.24%   -0.23%     
==========================================
  Files         967      967              
  Lines       33269    33411     +142     
==========================================
- Hits        17454    17453       -1     
- Misses      15815    15958     +143

Impacted Files	Coverage Δ
Rubberduck.Parsing/ComReflection/ComType.cs	`0% <ø> (ø)`	⬆️
Rubberduck.Parsing/ComReflection/ComParameter.cs	`0% <0%> (ø)`	⬆️
...bberduck.Parsing/Symbols/PropertySetDeclaration.cs	`47.06% <0%> (-14.48%)`	⬇️
Rubberduck.Parsing/ComReflection/ComMember.cs	`0% <0%> (ø)`	⬆️
Rubberduck.Parsing/ComReflection/ComCoClass.cs	`0% <0%> (ø)`	⬆️
...arsing/ComReflection/XmlPersistableDeclarations.cs	`33.33% <0%> (-2.38%)`	⬇️
...bberduck.Parsing/Symbols/PropertyLetDeclaration.cs	`47.06% <0%> (-14.48%)`	⬇️
Rubberduck.Parsing/ComReflection/ComField.cs	`0% <0%> (ø)`	⬆️
Rubberduck.Parsing/ComReflection/ComEnumeration.cs	`0% <0%> (ø)`	⬆️
...bberduck.Parsing/Symbols/PropertyGetDeclaration.cs	`41.07% <0%> (-15.03%)`	⬇️
... and 6 more

bclothier · 2018-07-16T01:08:41Z

Rubberduck.Parsing/ComReflection/ComField.cs

+                case VarEnum.VT_SAFEARRAY:
+                case VarEnum.VT_CARRAY:
+                case VarEnum.VT_ARRAY:
+                    tdesc = (TYPEDESC)Marshal.PtrToStructure(desc.lpValue, typeof(TYPEDESC));


Why not use Marshal.PtrToStructure<> instead of non-generic form and avoid the cast?

Copy paste. :-) These are all over the place, and I think it was because RD used to target 4.5. One of these days I'll find them all clean them up, but this at least keeps the usage fairly consistent.

bclothier · 2018-07-16T01:13:06Z

Rubberduck.Parsing/ComReflection/ComInterface.cs

+                Debug.Assert(length == 1);
+
+                _properties.Add(new ComField(info, names[0], property, index, DeclarationType.Property));
+                info.ReleaseVarDesc(varDescPtr);


if an exception is thrown before we get here, we'd have leaked a pointer, no? Maybe put in a try/finally? (or look at Max's DisposalActionContainer class for encapsulating dispose pattern on non-disposable objects.

@bclothier Good catch. Do we have a preference between try / finally and a set of wrapped IntPtrs that implement IDisposable? I personally find using blocks easier to read, but can go either way on it.

NVM, using also makes it clearer that the intent is for errors to be caught further down the stack.

bclothier · 2018-07-16T01:15:05Z

Rubberduck.Parsing/ComReflection/ComModule.cs

@@ -23,6 +24,8 @@ public class ComModule : ComType, IComTypeWithMembers, IComTypeWithFields
        private readonly List<ComField> _fields = new List<ComField>();
        public IEnumerable<ComField> Fields => _fields;

+        public IEnumerable<ComField> Properties => Enumerable.Empty<ComField>();


This feels wrong. Expression is OK for read-only properties but for writable properties, any writes would get lost, I think. Should be just a ordinary auto-property with Enumerable.Empty assigned at construction, I think.

Properties is read-only. The inheritance and interface structure in this namespace could probably use some work eventually, but in this case I put it on the IComTypeWithMembers interface because at this point I don't have any evidence that these are prohibited on Modules other than never having seen any. If I can conclusively determine from the MIDL spec that they'll never be there, I'll probably rearrange this.

bclothier · 2018-07-16T01:19:47Z

Rubberduck.Parsing/ComReflection/ReferencedDeclarationsCollector.cs

+                    continue;
+                }
+
+                if (item.IsReferenceType)


What about the unusual case where one has implemented all three forms of properties? Not sure if the long forms will get called into this method but if it does, then that if/else probably need to be rethought.

If the method is meant to deal only with the short form in where we can't specify the 3 types, then probably need a comment to call out the assumption here.

This is impossible. The short form declaration is basically the same as a public field, so in it's native form there isn't a set of accessors - only a read-only flag. The way they are used by the consumer is language specific. This may require more testing as to how the VB compiler handles them, but I can't make that determination until I find one that is actually a reference type.

comintern · 2018-07-16T01:53:23Z

WIP'd the PR to address potentially leaked pointers.

…clothier

…clothier). Addresses possible pointer leaks.

comintern · 2018-07-16T02:35:03Z

Addressed potential leaky pointers, no longer WIP. Props to @MDoerner - the DisposalActionContainer made that a cake walk, it's definitely going in my tool bag.

bclothier · 2018-07-16T04:27:52Z

I know this is already merged but there is one more consideration...

From the specs, we see that:

ppFuncDesc: MUST be set to a FUNCDESC structure (see section 2.2.42) that contains the data values associated with the specified member of the method or dispatch method table, or NULL if no such member exists.

It might be that the.NET implementation will throw exception in such cases but I would rather not assume. Therefore, before entering the using block with the DisposalActionContainer, we need to verify that the IntPtr isn't IntPtr.Zero. Otherwise, we would end up releasing a null pointer and you all know how C++ loves dem null pointers....

comintern · 2018-07-16T04:45:41Z

@bclothier - This line specifies the interface contract:

MUST be set to a FUNCDESC structure (see section 2.2.42) that contains the data values associated with the specified member of the method or dispatch method table, or NULL if no such member exists.

If no such member exists (and ppFuncDesc is set to NULL), this return value documentation applies:

The value of index did not specify the ordinal position of an element in the method table.

In that case, it returns TYPE_E_ELEMENTNOTFOUND, and the marshaller will respond by throwing a ComException (at least I think that one is a ComException - you can test it with Marshal.ThrowExceptionForHR(0x8002802B)). Basically any HRESULT other than S_OK will be thrown as a managed exception, so there isn't a need to explicitly check for a null pointer because the interface contractually will not do that unless it returns a non-zero HRESULT.

comintern added 6 commits July 15, 2018 15:33

Fix return types. Ref rubberduck-vba#4096, others.

b7f582e

Implement short-form properties.

4a2792d

Improve resolution of ComField types.

e8ae808

Tweaks for last prior commit, improve object resolution.

54263fc

Update serialized declarations in unit tests to reflect COM collectio…

290e7e2

…n changes.

Revert "Update serialized declarations in unit tests to reflect COM c…

b6b9d74

…ollection changes." This reverts commit 290e7e2.

bclothier reviewed Jul 16, 2018

View reviewed changes

comintern mentioned this pull request Jul 16, 2018

Serialized Declarations used by the unit test do not match the "production" declarations. #4191

Closed

comintern added the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Jul 16, 2018

Implement disposable resource pattern in ComReflection (nice catch @b…

aa7bfdb

…clothier). Addresses possible pointer leaks.

comintern removed the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Jul 16, 2018

retailcoder merged commit f05357f into rubberduck-vba:next Jul 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type name resolution and implement short form properties in COM collector. #4189

Fix type name resolution and implement short form properties in COM collector. #4189

comintern commented Jul 16, 2018

codecov bot commented Jul 16, 2018 •

edited

bclothier Jul 16, 2018

comintern Jul 16, 2018

bclothier Jul 16, 2018

comintern Jul 16, 2018

comintern Jul 16, 2018

bclothier Jul 16, 2018

comintern Jul 16, 2018

bclothier Jul 16, 2018

comintern Jul 16, 2018

comintern commented Jul 16, 2018

comintern commented Jul 16, 2018

bclothier commented Jul 16, 2018

comintern commented Jul 16, 2018

Fix type name resolution and implement short form properties in COM collector. #4189

Fix type name resolution and implement short form properties in COM collector. #4189

Conversation

comintern commented Jul 16, 2018

codecov bot commented Jul 16, 2018 • edited

Codecov Report

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

comintern commented Jul 16, 2018

comintern commented Jul 16, 2018

bclothier commented Jul 16, 2018

comintern commented Jul 16, 2018

codecov bot commented Jul 16, 2018 •

edited