Skip to content
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

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

Merged
merged 2 commits into from
Jun 10, 2022

Conversation

AArnott
Copy link
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 tmat, BertanAygun and wade0016 June 10, 2022 02:46
@codecov-commenter
Copy link

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
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
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
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
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