Skip to content

Disable navigate to decompiled sources for assemblies with SuppressIldasmAttribute#24187

Merged
sharwell merged 1 commit intodotnet:dev15.6.xfrom
sharwell:ilspy-suppressildasm
Jan 29, 2018
Merged

Disable navigate to decompiled sources for assemblies with SuppressIldasmAttribute#24187
sharwell merged 1 commit intodotnet:dev15.6.xfrom
sharwell:ilspy-suppressildasm

Conversation

@sharwell
Copy link
Contributor

@sharwell sharwell commented Jan 11, 2018

Customer scenario

A customer enables the Navigate to Decompiled Sources feature, accepts the terms for use, and navigates to a symbol defined in an assembly that has SuppressIldasmAttribute applied to it. The source code for the symbol is shown when only the metadata should be showing.

Bugs this fixes

Fixes #24175

Workarounds, if any

None.

Risk

Low.

Performance impact

Low.

Is this a regression from a previous update?

No.

Root cause analysis

The initial ILSpy integration (behind a feature flag and disabled by default) was not tested for this case. Manual testing revealed the bug.

How was the bug found?

Manual compliance testing.

Test documentation updated?

A manual test scenario was added.

@sharwell sharwell requested a review from a team as a code owner January 11, 2018 20:03
@sharwell sharwell added this to the 15.6 milestone Jan 11, 2018
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Tests?

var useDecompiler = allowDecompilation;
if (useDecompiler)
{
useDecompiler = !symbol.ContainingAssembly.GetAttributes().Any(attribute => attribute.AttributeClass.Name == nameof(SuppressIldasmAttribute));
Copy link
Member

Choose a reason for hiding this comment

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

Should this be checking fully qualified name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My original approach was to check the fully-qualified name. The more I though about it, it didn't seem to add anything.

@sharwell
Copy link
Contributor Author

Tests?

I don't have tests yet. I'm working on an approach to add them but this is a particularly important PR to take for Preview 4.

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Good, assuming we have something covering adding an automated test for this.

@jinujoseph
Copy link
Contributor

Approved to merge via link

@sharwell sharwell merged commit d482d81 into dotnet:dev15.6.x Jan 29, 2018
@sharwell sharwell deleted the ilspy-suppressildasm branch January 29, 2018 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants