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

Please allow [assembly: TypeForwardedTo(typeof(T))] when T is [Obsolete] #61264

Closed
rickbrew opened this issue May 12, 2022 · 2 comments · Fixed by #62127
Closed

Please allow [assembly: TypeForwardedTo(typeof(T))] when T is [Obsolete] #61264

rickbrew opened this issue May 12, 2022 · 2 comments · Fixed by #62127
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Milestone

Comments

@rickbrew
Copy link

I've been doing a lot of cleanup and refactoring in my app's codebase (Paint.NET) over the last several months (well, slowly over years, rapidly over the last 6 months). It's approximately 600,000 lines of code and has acquired a lot of cruft. A lot of that cruft can be deleted, as it's internal stuff.

However, some of that cruft is/was used by plugins. Sometimes because it was an official API that I was providing, other times because it was something I neglected to mark as internal (there's a whole story there... 🤦‍♂️). I cannot / will not delete these classes because I take backwards compatibility very seriously. I'm not interested in breaking someone's workflow because their favorite plugin was using an API that I want to sweep away, sometimes just for aesthetic reasons; I don't want to make it my user's/customer's problem. [Obsolete("...", true)] is a great tool for this.

I'm now finding that certain types of refactoring/cleanup are hitting another wall: I can move types to lower-layer assemblies by using [assembly: TypeForwardedTo(typeof(T))] and this works great ...except for obsolete types (with the error=true flag), because it results in a compile-time error that cannot be suppressed. This prevents me from taking a bunch of old classes out of e.g. PaintDotNet.Effects.dll and moving them all to PaintDotNet.Effects.Obsolete.dll. I want to do this to prevent new plugins from using them, and also to sweep them out of the way entirely (no Intellisense, no docs, etc.). But, old plugins would still continue to function (an absolute requirement).

In addition, the inability to use TypeForwardedTo(typeof(T)) on an obsolete type prevents me from doing other types of cleanup, such as dropping assembly references. If I can't move T out of its assembly, because it's obsolete, then I cannot prune any assembly references that T requires.

As an example, let's say that types T1 and T2 are in Assembly1. I want to make T1 obsolete and move it into Assembly1.Obsolete. I also want to move T2 into Assembly2, and not have Assembly1 reference Assembly2 at all (1Assembly1.Obsolete1, however, would reference Assembly2). Right now I can't do that, and it's causing some inflexibility in how I can refactor things.

There are two workarounds that I can think of for this right now. The first is to inject the [assembly: TypeForwardTo(...)] attributes as some kind of post-build event, e.g. using Mono.Cecil. That's way overkill and I shouldn't have to do that. The other is to use [Obsolete("...", false)], but that doesn't solve the problem of preventing new plugins from using those types (plugin authors will do this, they don't care 🤣 You should see the stuff I've seen in plugins ... dreadful, awful stuff!)

@rickbrew rickbrew added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels May 12, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 12, 2022
@CyrusNajmabadi
Copy link
Member

but that doesn't solve the problem of preventing new plugins from using those types (plugin authors will do this, they don't care

Obsolete(true) doesn't stop usage. It's easy to still use from things like your own obsolete scopes.

You should see the stuff I've seen in plugins ... dreadful, awful stuff!)

Then that's on them :-). You told them. They ignored (which is possible with error or warning). They get broken :-)

@rickbrew
Copy link
Author

rickbrew commented May 12, 2022

Obsolete(true) doesn't stop usage. It's easy to still use from things like your own obsolete scopes.

Nothing will be 100%, I'm not under the impression that is something that can be achieved :) You can certainly access obsolete elements via reflection, and probably other ways. But, there are also ways I can defend against that, such as blocking the plugin, (if it's a new release), or even having an allow-list of plugins that are allowed to load the new assembly that hosts the obsolete types (that is, only specific old plugins can use it; all plugins load into ALCs now and I can allow/block assemblies, but not types).

Then that's on them :-). You told them. They ignored (which is possible with error or warning). They get broken :-)

Sure, of course. But I can't inspect every plugin that is released, and sometimes a plugin is released that does something bad and it doesn't get caught until years later. In the meantime, it becomes popular and people depend on it for their use of the app. Then, me breaking the plugin inflicts pain on users/customers, not the plugin author, who often has wandered somewhere else in life and isn't maintaining the plugin.

I'm just looking for more tools to address the needs of refactoring and keeping the codebase clean, while maintaining compatibility and happy users.

@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label May 13, 2022
@jcouv jcouv added this to the Compiler.Next milestone May 13, 2022
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Jun 24, 2022
AlekseyTs added a commit to AlekseyTs/roslyn that referenced this issue Jun 24, 2022
@AlekseyTs AlekseyTs added the 4 - In Review A fix for the issue is submitted for review. label Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants