Add protected BaseResult() method to CallInfo.

[dtchepak]

Create CallInfo to calls that return results and expose GetBaseResult.
This gets messy to push the generic all the way through the code, so am
just using a cast in Returns extensions to handle this. This should be
safe as if we are in Returns<T> then the return value should be safe to
cast to a T.

Based off discussion here:
#622 (comment)

[dtchepak]

@zvirja This is my attempt at implementing your suggestions from #622. If this seems ok I can try to do something similar for When..Do if you like, but that will be trickier.

[alexandrnikitin]

@dtchepak I'm a bit out of context after absence and need more time to ramp up 😊 A couple of questions.
Can we limit it to Partial mocks only. So that in regular substitutes it's not possible to call base. At first look it makes sense not to mix them.
Would it be better if we make a breaking decision to not-call-base by default and expose that via API?

[dtchepak]

Can we limit it to Partial mocks only.

I don't think it is possible to distinguish partial mocks statically using our API. 😞

Would it be better if we make a breaking decision to not-call-base by default and expose that via API?

I'm still not sure on this all these years later! 😂 Leaving partial mocks out of this for the moment, I think it makes sense to be able to access the base value from within the Returns callback. At present the code (should) throw if there is no base implementation, so in cases where it does not make sense to call base people will immediately be informed via their test.

[dtchepak]

@tpodolak Will this change to use CallInfo<T> cause any problems for Analyzers?

[macsux]

Would like to see this merged in as I need this functionality too

[tpodolak]

@tpodolak Will this change to use CallInfo<T> cause any problems for Analyzers?

I think it might. Analyzers do some check against CallInfo type. For instance, they analyze ArgAt method usage only if this method is declared in CallInfo type. CallInfo will not fall into that category (at least from perspective of current analyzers codebase). I will try to double check this week, but I am quite sure that some diagnostics might not be reported

[dtchepak]

@tpodolak CallInfo<T> inherits from CallInfo; not sure if that will help?

[tpodolak]

@tpodolak Will this change to use CallInfo<T> cause any problems for Analyzers?

I think it might. Analyzers do some check against CallInfo type. For instance, they analyze ArgAt method usage only if this method is declared in CallInfo type. CallInfo will not fall into that category (at least from perspective of current analyzers codebase). I will try to double check this week, but I am quite sure that some diagnostics might not be reported

@dtchepak I've built your branch and run analyzers against that and as expected, callInfo warnings are gone

• NSubstitute 4.2.2
  
• CallInfo version
  
  

@tpodolak CallInfo inherits from CallInfo; not sure if that will help?

Thanks, I've seen that, however this still breaks current CallInfo alayzers. I will fix analyzers part once this is merged and published to NuGet or some private feed

[dtchepak]

Thanks @tpodolak . Is it possible to support both? Or will different NSub versions require different Analyzer versions?

[dtchepak]

@zvirja Are you OK with this change? If you don't have time to look at it please let me know. 😄

[tpodolak]

Thanks @tpodolak . Is it possible to support both? Or will different NSub versions require different Analyzer versions?

It is possible to support both versions with just one version of analyzers. Here are necessary chagnes nsubstitute/NSubstitute.Analyzers#158

[zvirja]

Hi @dtchepak! Hope you are doing fine and are not annoyed too much that I'm not replying timely 😖

I actually have a bit of concerns regarding the newly introduced CallInfo<T>. When working on nullability annotations I found that the separation between internal (core) and external parts of NSubstitute is not clear. Sometimes we have public-facing API which are returning low-level Core entities.

In order to enhance that I actually was planning to revisit it the next major version. The idea was that we e.g. have public interfaces for things like CallInfo, Call and never return exact implementations.

The motivation is that we could modify Core more freely without worrying to affect public surface. Also we could more clearly annotate code with nullability attributes - public surface not annotated, private - annotated entirely.

Same for consumption - if you consume anything from Core namespace - expect things to be more fragile and volatile.

Not sure whether you share my vision on this. Library is really small, so such kind of things might be an overkill. But somehow I see a beauty and usefulness in that, as we are just a few tiny steps from there.

[dtchepak]

@zvirja

Hope you are doing fine and are not annoyed too much that I'm not replying timely

Of course not annoyed! We're all volunteers here, I am grateful for any reply!

In order to enhance that I actually was planning to revisit it the next major version. The idea was that we e.g. have public interfaces for things like CallInfo, Call and never return exact implementations.

Would you be happier if I change this PR to expose interfaces for these? Alternatively we could take this as-is (if you are happy enough with the implementation) and do the larger change to interfaces in a separate issue?

We should also be mindful that any change along these lines will probably make a lot of work for the @tpodolak and the Analyzers project.

[zvirja]

Hi @dtchepak,

Would you be happier if I change this PR to expose interfaces for these?

Yes, I think it would be nice to start with interfaces approach for new features, so later we could rework other features to catch-up. This way we can also see how it looks like.
I would put interface somewhere outside of Core, but the implementation could be in Core somewhere.

[dtchepak]

@zvirja I've attempted to extract ICallInfo into root namespace. Can you please re-review?

I'm a bit concerned about the ICallInfo.ForCallReturning<T> but wasn't sure how best to do this. (I think this is better than casting to the internal CallInfo type though.)

[zvirja]

@dtchepak I've gave it a brief look and before we proceed further, I can see that we introduce a lot of breaking changes to API (as use now use interface instead of implementation). I'm afraid that it could break other libraries (like AutoFixture) which might depend on these APIs. Should we then consider branching out v5 and releasing a new major version with this?

Then we could add a couple of other improvements we planned to add.

[dtchepak]

Thanks @zvirja. We are fine for next release to be 5.0.0 I think. We tend not to branch for that; develop will just be tracking 5.x now. I'm happy to put more process around this if you prefer?