-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Suppress dynamic access to instance indexers off of types. #11677
Conversation
@dotnet/roslyn-compiler Please review Update 3 fix. CC @MattGertz, @jaredpar For ask mode approval. |
Ping. @dotnet/roslyn-compiler Please review Update 3 fix. CC @MattGertz, @jaredpar For ask mode approval. |
Pending CRs. |
|
||
[ClrOnlyFact(ClrOnlyReason.Ilasm)] | ||
[WorkItem(204561, "https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=204561&_a=edit")] | ||
public void Bug204561_01() |
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.
Perhaps name the tests SuppressDynamicIndexerAccess
or something similar.
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 do.
LGTM |
@dotnet/roslyn-compiler Please review Update 3 fix, I need a second sign-off. |
} // end of class WithIndexer | ||
"; | ||
|
||
MetadataReference reference = CompileIL(iLSource, appendDefaultHeader: true, embedInteropTypes: false); |
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.
Can an assembly of this structure only be created in IL? Just asking because if it could also be created in VB it may be interesting to have a test with VB code to ensure reasonable interop.
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 prefer to test from IL in this case. As you can see, there is no interop with indexed property in this case and that is the scenario we are interested about.
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.
Yeah, I suppose producing a binding error isn't interop. Sounds good.
LGTM |
Renamed unit-tests. |
Fixes some of the scenarios for https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=204561&_a=edit.