Make sealed/internal all classes that could be sealed/internal

[Romfos]

Changes:

• Make internal all classes that could be internal
• Make sealed all classes that could be sealed

note 1: technically this is breaking change, but de-facto no any changes required for 99% of customers

note 2: some "Core" types are still public (but could be sealed) because they are used in public interfaces and make them internal could be a "more real and impactful" breaking change. But we can make a part 2 for this in future, see #868

Related #830

[zvirja]

@Romfos What is the motivation for this change? What is it solving? It probably doesn't have any practical implication, but then if it doesn't - why to do it?

I remember an argument about sealed types being a bit faster, as calls could be de-virtualized. But it only happens for virtual calls and we don't have those.

Of course we have an argument of code design, but then we should just make types internal. That would make way more sense.

I feel that typical .NET code is not having sealed, as least that's from my experience. You maybe seal attributes, or sometimes when you have inheritance and virtual members used in constructor - but that's it. When I see sealed, for me it's a signal that there is something special about the type and I have to pay attention to it. So I would rather be confused. If we look at arbitrary .NET code (e.g. here) - it's not sealed. System.String is - but because it's very special.

I feel that this change will just force us to make declaration of types more verbose for no practical reasons at all. And if somebody is extending NSubstitute (I've done that many times in my projects, because I know its source code 😉), then we are creating a potential inconvenience (even though I personally always favor composition over inheritance).

I do appreciate you putting your effort in making the code being nicer. You spend time and I don't these days, so it's totally fare that you have a higher "privilege" to decide 😉 I just give my 5 cents here because I feel that this change makes code more verbose and inconvenient for no good reasons. Sealing everything is an industry standard as much as I am aware of. And having to seal every single class you create in the future - it's just annoying to me.

[Romfos]

Hello,

Goal of this change: Segregate "public api" and other internal stuff
Motivation: Make future support and changes easier and version policy more clear

1. Extensibility based on interfaces

Now all classes are public and are not sealed
Now user code could inherit form any NSubstitute class and make dependency on behavior of them.
Any change in NSubstitute classes now technically is a breaking change (even if it will affect small amount of users).
Would be nice to control how and when user can depend on NSubstitute classes and control it

For example, some time ago you created a list of possible improvements #551

There are some improvments like:

• Make RouteAction a read-only struct to optimize performance
• Convert to a record once we drop .NET Standard 1.3 support: ArgumentMatchInfo, Quantity, Make SubsitutionContext constructor private, so it can be instantiated via container only.
• Make NSubstitute.Core.Argument a read-only struct, as it's a thin wrapper on top of ICall and index.

All these changes technically are breaking change in current arch.

In case of internal classes we can control and make changes like that absolutely safe, because public classes are not changed

Proposed extensibility strategy:

1. If class is not client visible - lets try to make it private/internal and sealed
2. If we cannot make class internal or private - lets try make it sealed
3. Extensibility via container and interfaces. Each interface is a kind of extensibility point

2. Transparent version policy:

major change:

• remove\modify for public api

minor change:

• add new public api
• add\remove\modify for internal stuff

patch version:

• hotfix only

This change allow on code review stage estimate impact for proposed change

Mostly this is not relevant to NSubstitute, but in general good to know:

• In modern dotnet sealed also help Jit to generate more optimal code:
  https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-6/#“peanut-butter”

• sealed also helps tooling like visual studio perform better in features like "Find usages", intellicode and other code analysis. In case of sealed class, tooling will not continute search for sub classes, and for big projects visual studio (including tooling like a resharper)

• Yes, dotnet team don't use sealed in BCL for old public types. Motivation is simple - they want to avoid a breaking change.

At the same time for NEW types (that are added in .NET 6+). They are also using sealed. At the same time they have segregation like: code that should be performant (like span api) and places when they don't care. It depends

https://github.com/dotnet/runtime/blob/9d9bbcc450bdf7643c9c75faff6fae9a1276f832/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/Pipe.cs

https://github.com/dotnet/runtime/blob/9d9bbcc450bdf7643c9c75faff6fae9a1276f832/src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/DefaultFrozenDictionary.cs

https://github.com/dotnet/runtime/blob/9d9bbcc450bdf7643c9c75faff6fae9a1276f832/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableList_1.cs

Anyway we can continue current approach and still do all types as public, it just make support more tricky

[dtchepak]

Appreciate @zvirja's concerns about limiting extensibility, but happy to leave the final call to @Romfos. Perhaps as a compromise we can focus on classes that almost definitely have no resuse value (internals like CastleForwardingInterceptor, rather than handlers/formatters?)

Perhaps should also specify action on BreakingChanges.md if people get stuck with this (probably they need to raise issue with us to restore public visibility).

[dtchepak]

chore: duplicate entry

[dtchepak]

thought: there might be an argument for keeping this one public? If someone is implementing their own arg matchers they may want to reuse formatting. WDYT?

[dtchepak]

question: not sure if it is worth keeping handlers public in case people want to extend with their own routes?

[Romfos]

sorry for delay. Looks like this pr require more time to work

[dtchepak]

Hi @zvirja , any additional thoughts on this?
I'm not worried about the sealed changes, but I do worry we'll frustrate devs that extend nsub by changing visibility to internal.

Given your experience with extending nsub, are there any particular classes we really should keep public?

[304NotModified]

but I do worry we'll frustrate devs that extend nsub by changing visibility to internal.

IMO there is too much public visible and that makes things hard to change. Or we cannot change it, or we have to bump many times the major version.

For example, https://github.com/nsubstitute/NSubstitute/blob/main/src/NSubstitute/Core/CallFactory.cs, is that really needs to be public?

Of course we cannot make everything internal, it's a balance trick. It's a bit hard, as some part aren't documented for public usage and then you have to guess.

IMO it is not a bad approach, when doubting, to make it internal and wait for feedback.

It's also in benefit for the end-user that we could do breaking changes in internal code :)

[304NotModified]

@dtchepak could you please merge this PR?

[Romfos]

@dtchepak Could you please help to align common vision for NSubstitute future:

1. Do we want this change or better postpone this change for now?
2. if we ok with this change - what should stay public?

[304NotModified]

It's approved so we could merge it, right?

[zvirja]

What about another compromise, when we keep this public, but move them let's say to internal namespace? Then we could be very transparent, that any change there will not be a subject to SemVer and could change any time.

My concern is extensibility. Often - adhoc extensibility, when I could hack some things quickly and use that in the project. You never known what you need. Even though I would admit that I do that rarely.

As for the sealed - well, I still don't see them much in real life, maybe only on attributes. For me it's noice and just unnecessary verbose modifier. I see the arguments above and they are perfectly reasonable - just not enough reason for me personally. We are just a mocking library - not BCL, we don't have to be strict.

I think we are solving the problem we never had in practical terms. I cannot remember people coming and complaining that we broke something and they actively used it. We don't break things often either and we are not too shy to just raise a major version for that.

I do again want to stress that I do appreciate the effort you do. It's not a game changer to me, so follow either way and I'll adjust. My personal opinion - to do that. But I am not an active maintainer - so take it with a multiplier 😊

[304NotModified]

I think we are solving the problem we never had in practical terms. I cannot remember people coming and complaining that we broke something and they actively used it. We don't break things often either and we are not too shy to just raise a major version for that.

I think probably soms large refactors are skipped because of the public interface. And also as user, it's confusing there is so much public.

My concern is extensibility. Often - adhoc extensibility, when I could hack some things quickly and use that in the project. You never known what you need. Even though I would admit that I do that rarely.

User could always code the source code. And also, with extensibility it more useful to know the goal instead of the solution. So if we get any requests, we should ask for the goal (and not the solution)

What about another compromise, when we keep this public, but move them let's say to internal namespace?

I could live with it, but I think it's not the best choice. Because of the goal/solution topic

[dtchepak]

Could you please help to align common vision for NSubstitute future:

• Do we want this change or better postpone this change for now?
• if we ok with this change - what should stay public?

I think both @Romfos and @zvirja have very valid points. As @zvirja points out, as he and I aren't able to be as active with contributions at the moment I think it is ok for @Romfos to have the final decision (taking into account the valid points raised on both sides of the discussion).

For me, I think this compromise sounds good:

• sealing types is fine and a positive change. The verbosity doesn't bother me at all.
• I still have concerns about limiting extensibility, so I like @zvirja's compromise of using internal namespace and making clear that types there will not be subject to semver and to "extend at your own risk".

I've approved this PR and it's fine for it to be merged without changes. @Romfos please merge when you are happy with it. I've invited @304NotModified to have merge access as well.

Even if we all have different opinions on how to get there, I know we all have our users' best interests in mind and if later on we find out another option seems better then we can always change it and bump up the version number. ❤️

[304NotModified]

@Romfos

Why is this pr closed? (And a force push afterwards? )

[Romfos]

looks like git automatically closes PR if i do sync with master branch in my fork

@304NotModified According to @dtchepak comment we need different implementation.

Sorry little bit busy at this moment of time. I will comeback with new changes later or someone else could implement it

[304NotModified]

I've invited @304NotModified to have merge access as well.

@dtchepak I don't have merge access nor I can't reopen this pr.

[Romfos]

actually we don't need this changes, we need make new

[dtchepak]

I've invited @304NotModified to have merge access as well.

@dtchepak I don't have merge access nor I can't reopen this pr.

I think I've fixed this now 🤞