-
Notifications
You must be signed in to change notification settings - Fork 468
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
Implement analyzer and code-fixers for DynamicInterfaceCastableImplementation analyzers #5129
Conversation
…ng to write an implementation in VB (which is impossible to do correctly).
…into idic-analyzer
Codecov Report
@@ Coverage Diff @@
## release/6.0.1xx #5129 +/- ##
==================================================
Coverage 95.61% 95.62%
==================================================
Files 1240 1244 +4
Lines 285455 286799 +1344
Branches 17134 17213 +79
==================================================
+ Hits 272951 274248 +1297
- Misses 10193 10232 +39
- Partials 2311 2319 +8 |
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementationTests.cs
Outdated
Show resolved
Hide resolved
async ct => await SealMemberDeclaredOnImplementationType(declaration, context.Document, context.CancellationToken).ConfigureAwait(false), | ||
equivalenceKey: nameof(MicrosoftNetCoreAnalyzersResources.SealMethodDeclaredOnImplementationType)), | ||
diagnostic); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else throw NotSupportedException or something that ensures the code path isn't reachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think silently falling through here is ok unless the pattern in the repo otherwise is to throw.
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Show resolved
Hide resolved
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
...s/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementationTests.cs
Show resolved
Hide resolved
<value>Implement inherited interfaces</value> | ||
</data> | ||
<data name="InterfaceMethodsMissingImplementationDescription" xml:space="preserve"> | ||
<value>Types attributed with 'DynamicInterfaceCastableImplementationAttribute' act as an interface implementation for a type that implements the 'IDynamicInterfaceCastable' type. As a result, it must provide an implementation of all of the methods defined in the inherited interfaces, because the type that implements 'IDynamicInterfaceCastable' will not provide them otherwise.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is IDynamicInterfaceCastable
is a must to use DynamicInterfaceCastableImplementationAttribute
? Any separate analyzer work for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDynamicInterfaceCastable
is a must-use type related to the DynamicInterfaceCastableImplementationAttribute
type. But since IDynamicInterfaceCastable
will be implemented by a different type, so there's no analyzer work for that.
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
@dotnet/roslyn-analysis just want to make sure that reviewing this is on the backlog as it's been green in CI and waiting for review for 27 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some style problems. In general I think this repo uses var
when possible. I marked a few but realized a lot of the same comment would get tedious.
There also may be some missing paths for testing.
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Show resolved
Hide resolved
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
...soft.NetCore.Analyzers/InteropServices/CSharpDynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
…ers into idic-analyzer
@sharwell is there an ETA on when this will be reviewed? Last review pass was two weeks ago and the only changes since then were resolving merge conflicts. |
…into idic-analyzer
d463949
to
d558915
Compare
<value>All members declared in parent interfaces must have an implementation in a DynamicInterfaceCastableImplementation-attributed interface</value> | ||
</data> | ||
<data name="MembersDeclaredOnImplementationTypeMustBeSealedDescription" xml:space="preserve"> | ||
<value>Since a type that implements 'IDynamicInterfaceCastable' will conventionally not specify that it implements a 'DynamicInterfaceCastableImplementationAttribute'-attributed type, only the public interface type, virtual interface method lookup will fail even if the method has an implementation. As a result, all members on 'DynamicInterfaceCastableImplementation'-attributed types should be 'sealed'.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... will conventionally not specify that it implements a 'DynamicInterfaceCastableImplementationAttribute'-attributed type, only the public interface type, virtual interface method lookup will fail even if the method has an implementation ...
I don't understand this wording. Can you suggest a rephrasing to iterate on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<value>Since a type that implements 'IDynamicInterfaceCastable' will conventionally not specify that it implements a 'DynamicInterfaceCastableImplementationAttribute'-attributed type, only the public interface type, virtual interface method lookup will fail even if the method has an implementation. As a result, all members on 'DynamicInterfaceCastableImplementation'-attributed types should be 'sealed'.</value> | |
<value>Since a type that implements 'IDynamicInterfaceCastable' will not specify in source or in the `IDynamicInterfaceCastable` implementation that it implements a 'DynamicInterfaceCastableImplementationAttribute'-attributed type, virtual interface method lookup will fail even if the method has an implementation. As a result, all members on 'DynamicInterfaceCastableImplementation'-attributed types should be 'sealed'.</value> |
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Show resolved
Hide resolved
...s/Core/Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
.../Microsoft.NetCore.Analyzers/InteropServices/DynamicInterfaceCastableImplementation.Fixer.cs
Outdated
Show resolved
Hide resolved
9a131d5
to
be6cae2
Compare
…and require manual intervention on properties and events) since even in the sealed case, there are scenarios where Roslyn emits callvirt to sealed or private instance members.
…into idic-analyzer
…into idic-analyzer
…ers into idic-analyzer
CI failure is #5381 and is unrelated to this PR (it's failing in the release/6.0.1xx branch without this change). |
Implement analyzers and code fixes as per dotnet/runtime#41529
Fixes dotnet/runtime#41529