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

Analyzer to flag a non-public class if it is not marked as sealed #23426

Closed
alrz opened this issue Nov 28, 2017 · 7 comments
Closed

Analyzer to flag a non-public class if it is not marked as sealed #23426

alrz opened this issue Nov 28, 2017 · 7 comments

Comments

@alrz
Copy link
Member

alrz commented Nov 28, 2017

No description provided.

@alrz alrz changed the title Analyzer to flag a non-public class it is not marked as sealed Analyzer to flag a non-public class if it is not marked as sealed Nov 28, 2017
@benaadams
Copy link
Member

benaadams commented Nov 28, 2017

Only relevant if it has override methods?

@alrz
Copy link
Member Author

alrz commented Nov 28, 2017

@benaadams

Why? In my experience, sealed types generally have better isinst performance so I think this should be applied regardless of the presence of overridden methods. This is, however, only relevant if it hasn't any derived classes.

Further, we could use an option to produce warning if (public) types are not explicitly marked as non-sealed (with an attribute).

@benaadams
Copy link
Member

Further, we could use an option to produce warning if (public) types are not explicitly as non-sealed (with an attribute).

Basically you are saying classes should always be sealed by default?

Non-sealed classes are very useful to side-car data which you can't do with composition; e.g. pass type to api, which then api passes elsewhere to get the data back out - without creating a global(ish) dictionary on with the object as key.

sealed types generally have better isinst performance

If you are using isinst its a direct example of side-carring data on the object type (or the type's parent/hierarchy)

@alrz
Copy link
Member Author

alrz commented Nov 28, 2017

Basically you are saying classes should always be sealed by default?

This is what I'm trying to say:

private class HasNoDerivedTypes {} // warning: the type can be sealed

public class HasNoDerivedTypes {} // optional warning
[NonSealed] public class HasNoDerivedTypes {} // explicit suppression 

Note: this is an IDE request.

@CyrusNajmabadi
Copy link
Member

I see. It sounds like you want a suggestion that says to mark a class as sealed iff:

  1. it has private accessibility.
  2. it is not subclassed within the assembly.

If so, that seems reasonable. Sounds like it would go in the roslyn-analyzers repo though.

Note: afaict, there is no way to do this with the current analyzer system. You'd need a CompilationEndAction so you could ensure that there were no subclasses. But CompilationEndActions do not run by defualt for users. Tagging @heejaechang

@jinujoseph
Copy link
Contributor

Tagging @mavasani , @dotnet/roslyn-analysis

@jinujoseph jinujoseph added this to the Unknown milestone Nov 29, 2017
@Youssef1313
Copy link
Member

Duplicate of dotnet/runtime#49944

This analyzer will be implemented in dotnet/roslyn-analyzers repository, except for the "optional warning" part for public APIs. This is very unlikely to be useful.

@jinujoseph I believe this issue can be closed.

@333fred 333fred closed this as completed Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants