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

Obsolete SuppressIldasmAttribute #40622

Closed
teo-tsirpanis opened this issue Aug 10, 2020 · 7 comments · Fixed by #50951
Closed

Obsolete SuppressIldasmAttribute #40622

teo-tsirpanis opened this issue Aug 10, 2020 · 7 comments · Fixed by #50951
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Aug 10, 2020

Edit by @GrabYourPitchforks: This issue tracks only obsoleting the attribute, not any other ildasm.exe work.

namespace System.Runtime.CompilerServices
{
    [Obsolete("SuppressIldasmAttribute has no effect in .NET 6.0+ applications.")] // NEW attribute
    [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Module)]
    public sealed class SuppressIldasmAttribute : Attribute
    {
        public SuppressIldasmAttribute() { }
    }
}

(What text do we even put there? "This attribute never made any sense to begin with and has no valid use case."? I've based the text proposed above on the DisablePrivateReflectionAttribute obsoletion text already in 6.0.)


It has no valid use case, the security against reverse engineering it provides is nonexistent and there are tons of tools that ignore it.

My proposal is to deprecate it per #33360 and modify ILDasm to stop honoring that attribute.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.ComponentModel.DataAnnotations untriaged New issue has not been triaged by the area owner labels Aug 10, 2020
@ghost
Copy link

ghost commented Aug 10, 2020

Tagging subscribers to this area: @ajcvickers
See info in area-owners.md if you want to be subscribed.

@vcsjones
Copy link
Member

Related / duplicate: #13341

@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 7, 2020
@GrabYourPitchforks GrabYourPitchforks changed the title Idea: deprecate the SuppressIldasmAttribute Obsolete SuppressIldasmAttribute Apr 7, 2021
@dotnet dotnet unlocked this conversation Apr 7, 2021
@GrabYourPitchforks
Copy link
Member

Reopening this issue, as we need a work item to track obsoleting this specific API.

@GrabYourPitchforks GrabYourPitchforks added api-ready-for-review API is ready for review, it is NOT ready for implementation area-System.Runtime.CompilerServices and removed untriaged New issue has not been triaged by the area owner labels Apr 7, 2021
@ghost ghost added this to Needs triage in Triage POD for Meta, Reflection, etc Apr 7, 2021
@GrabYourPitchforks GrabYourPitchforks added this to the 6.0.0 milestone Apr 7, 2021
@GrabYourPitchforks GrabYourPitchforks self-assigned this Apr 7, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 8, 2021
@dotMorten
Copy link

I made a comment over here why I think this is a bad idea: #13341 (comment)

@buyaa-n buyaa-n moved this from Needs triage to v-Next in Triage POD for Meta, Reflection, etc Apr 12, 2021
@bartonjs
Copy link
Member

bartonjs commented May 18, 2021

Video

Approved as proposed, but use the next available diagnostic ID.

namespace System.Runtime.CompilerServices
{
+   [Obsolete("SuppressIldasmAttribute has no effect in .NET 6.0+ applications.")]
    [AttributeUsage(AttributeTargets.Assembly | AttributeTargets.Module)]
    public sealed class SuppressIldasmAttribute : Attribute
    {
        public SuppressIldasmAttribute() { }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 18, 2021
Triage POD for Meta, Reflection, etc automation moved this from v-Next to Done May 19, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 19, 2021
@dotMorten
Copy link

@bartonjs What is the alternative for this feature then ?
image

@GrabYourPitchforks
Copy link
Member

There is no alternative. During API review we briefly touched on whether we want to rip out support entirely or require a "/force" switch. The conclusion was that .NET (which includes the BCL and SDK tools such as ildasm.exe) should simply drop all support.

Nothing in this proposal or in the merged PR prevents other disassemblers (such as Visual Studio) from continuing to honor the attribute. Those disassemblers can abort the operation, display a warning, or implement any other behavior that might be appropriate for their scenarios.

@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants