Skip to content

Fix visibility checks when generating proxies based on internal interfaces#804

Merged
AArnott merged 2 commits intomicrosoft:mainfrom
AArnott:fix789
Jun 10, 2022
Merged

Fix visibility checks when generating proxies based on internal interfaces#804
AArnott merged 2 commits intomicrosoft:mainfrom
AArnott:fix789

Conversation

@AArnott
Copy link
Copy Markdown
Member

@AArnott AArnott commented Jun 10, 2022

.NET Reflection doesn't report members as part of a derived interface. One must ask each interface up the type hierarchy for its members. In #789, an internal interface was defined in the local assembly and declared no members, so we thought no skip visibility attribute was necessary. Once we add the type hierarchy traversal, the additional assembly is discovered and the skip visibility attribute added, allowing the new test to pass.

Fixes #789

@AArnott AArnott added this to the v2.12 milestone Jun 10, 2022
@AArnott AArnott requested review from BertanAygun, tmat and wade0016 June 10, 2022 02:46
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 10, 2022

Codecov Report

Merging #804 (342a4a6) into main (c3a4664) will decrease coverage by 0.08%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #804      +/-   ##
==========================================
- Coverage   91.89%   91.81%   -0.09%     
==========================================
  Files          61       61              
  Lines        5367     5375       +8     
==========================================
+ Hits         4932     4935       +3     
- Misses        435      440       +5     
Impacted Files Coverage Δ
src/StreamJsonRpc/SkipClrVisibilityChecks.cs 92.63% <83.33%> (-1.63%) ⬇️
src/StreamJsonRpc/MessageHandlerBase.cs 95.18% <0.00%> (-1.21%) ⬇️
src/StreamJsonRpc/JsonRpc.cs 93.77% <0.00%> (-0.40%) ⬇️
src/StreamJsonRpc/HeaderDelimitedMessageHandler.cs 88.75% <0.00%> (+0.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a4664...342a4a6. Read the comment docs.

@AArnott AArnott enabled auto-merge June 10, 2022 02:56
}

[Fact]
public void Tomas_Internal()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to update the test name to describe the scenario better?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't my name self-explanatory? ;)

{
for (TypeInfo? t = startingPoint.GetTypeInfo(); t is not null && t != typeof(object).GetTypeInfo(); t = t.BaseType?.GetTypeInfo())
{
yield return t;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to recurse into interfaces implemented by these base types or do we only care about the interfaces declared directly by the original starting point?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I copied this code from another place in the library without thinking. This code will never be hit because proxies are always based on interfaces, not classes. I'll delete.

@AArnott AArnott merged commit b97ecf9 into microsoft:main Jun 10, 2022
@AArnott AArnott deleted the fix789 branch June 10, 2022 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ServiceJsonRpcDescriptor.JsonRpcConnection.ConstructRpcClient<T> fails to create an instance

4 participants