Skip to content

Fix broken comparison of the generic methods - #990

Merged
dtchepak merged 2 commits into
nsubstitute:mainfrom
zvirja:fix-828-distinguish-generic-overloads
Aug 10, 2026
Merged

Fix broken comparison of the generic methods#990
dtchepak merged 2 commits into
nsubstitute:mainfrom
zvirja:fix-828-distinguish-generic-overloads

Conversation

@zvirja

@zvirja zvirja commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #828

Currently we compare generic methods by names which is wrong in case we e.g. have shadowing via new. Do proper generic definition comparison, so that we preserve methods identity. Added tests to validate the scenarios.

zvirja and others added 2 commits August 9, 2026 17:33
Argument matchers for generic methods failed to tell apart two distinct
overloads that share a name but differ by their generic method definition
(e.g. a base interface method and a `new` reimplementation in a derived
interface with a narrower generic constraint).

CanCompareGenericMethods identified generic overloads by name only, so a
specification built for the base overload was considered comparable to a
call on the derived overload. Combined with the assignability check on the
closed generic arguments, a base-typed matcher (Arg.Any<IOutBase>) would
match a call to the derived overload closed over a subtype (IOutDerived),
producing false matches when verifying calls and resolving return values.

Compare the generic method definitions instead of the names. This still
allows a single method to match calls closed over different (assignable)
type arguments, while keeping genuinely distinct overloads apart.

Fixes nsubstitute#828

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a regression scenario alongside the nsubstitute#828 field report: overriding a
generic method (as opposed to 'new' hiding it) keeps a single method slot,
so the spec and the recorded call resolve to the same generic method
definition whether the substitute is used through the derived or the base
type. This ensures comparing generic method definitions does not tighten
matching too far for legitimate overrides.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@zvirja
zvirja requested a review from dtchepak August 9, 2026 19:14
@dtchepak
dtchepak merged commit a466b7a into nsubstitute:main Aug 10, 2026
4 checks passed
@dtchepak

Copy link
Copy Markdown
Member

@zvirja happy for me to release current main as 6.2.0?

Should i set up the nuget-push environment to require a reviewer prior to it running?

@zvirja
zvirja deleted the fix-828-distinguish-generic-overloads branch August 10, 2026 14:20
@zvirja

zvirja commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

I am creating 2 more PRs and we could go after that.

Should i set up the nuget-push environment to require a reviewer prior to it running?

I am not sure I follow what you mean and exactly it happens. But I would say - no, if one creates a tag, we assume one knows what they are doing, so it's okay :)

@dtchepak

dtchepak commented Aug 10, 2026

Copy link
Copy Markdown
Member

Something like this:

image

if one creates a tag, we assume one knows what they are doing, so it's okay

You seem to be forgetting I am still involved with this project and have access to create tags 😅 🤣

I take your point though; definitely less friction to just tie it to releases, I just tend to be (overly?) cautious with this stuff. 😄

@zvirja

zvirja commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Something like this:

Wow, I didn't even know you could review those things 😮 So many projects I work alone on, so nice to learn 😅

I take your point though; definitely less friction to just tie it to releases, I just tend to be (overly?) cautious with this stuff. 😄

I wouldn't be much worried. Worst case scenario - we can always unlist things and bump versions to fix. I had to fix the mess so many times in my life until now, so I wouldn't worry at all. But of course we shall be reasonable in our actions 😅

@zvirja

zvirja commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@dtchepak Let's proceed with merge once me merge params. We have a huge issue with generics support, especially after we got Arg.AnyType - it allows some significant violation scenarios. Especially if you start playing with covariance/contravariance. I wanted to quickly fix it, but apparently the journey is far more complicated. I may finalize it later, but it shall come either as a part of next 6.x, or we even releaes 7.x if there are required breaking changes in how generics are handled.

tknightnd pushed a commit to NewDayTechnology/benchmarkdotnet.analyser that referenced this pull request Aug 12, 2026
Updated
[Microsoft.Extensions.DependencyInjection](https://github.com/dotnet/dotnet)
from 10.0.10 to 10.0.11.

<details>
<summary>Release notes</summary>

_Sourced from [Microsoft.Extensions.DependencyInjection's
releases](https://github.com/dotnet/dotnet/releases)._

No release notes found for this version range.

Commits viewable in [compare
view](https://github.com/dotnet/dotnet/commits).
</details>

Updated [NSubstitute](https://github.com/nsubstitute/NSubstitute) from
6.0.0 to 6.2.0.

<details>
<summary>Release notes</summary>

_Sourced from [NSubstitute's
releases](https://github.com/nsubstitute/NSubstitute/releases)._

## 6.2.0

# NSubstitute v6.2.0

This release improves generic call matching (#​989, #​974, #​990).
Thanks to @​zvirja and @​JMolenkamp for fixes this.
We've also switched to using trusted nuget publishing linked to GitHub
Releases. (#​987, @​zvirja)

## What's Changed

* Add trusted nuget publish by @​zvirja in
nsubstitute/NSubstitute#987
* fix: Stop using return-type assignability for generic call matching by
@​zvirja in nsubstitute/NSubstitute#989
* Remove skip-duplicate option from NuGet push command by @​zvirja in
nsubstitute/NSubstitute#988
* Add tag trigger for release pipeline by @​zvirja in
nsubstitute/NSubstitute#991
* Fix broken comparison of the generic methods by @​zvirja in
nsubstitute/NSubstitute#990
* Support C# 13 params collections in argument matching by @​zvirja in
nsubstitute/NSubstitute#993
* Add tests for nullable Task by @​zvirja in
nsubstitute/NSubstitute#994

**Full Changelog**:
nsubstitute/NSubstitute@v6.1.0...v6.2.0

## 6.1.0

# NSubstitute v6.1.0

This release reverts nullability support introduced in 6.0. This change
caused a number of problems for callers (#​973, #​976), and trying to
resolve these (#​976) revealed the need for more fundamental changes to
NSubstitute to effectively support this. As a result, nullability is
again disabled for public API. Huge thanks to @​jdb0123, @​Romfos, and
@​Moha-sami for their PRs addressing this, and thanks for all those who
raised issues. We also humbly award @​zvirja with one unlimited "I told
you so" pass to use as he sees fit.

## What's Changed

* Add new argument matcher for comparing objects by reference by
@​Wer-Wolf in nsubstitute/NSubstitute#963
* Revert nullability changes in
nsubstitute/NSubstitute#985

## New Contributors
* @​Wer-Wolf made their first contribution in
nsubstitute/NSubstitute#963

**Full Changelog**:
nsubstitute/NSubstitute@v6.0.0...v6.1.0

Commits viewable in [compare
view](nsubstitute/NSubstitute@v6.0.0...v6.2.0).
</details>

Updated [NuGet.Common](https://github.com/NuGet/NuGet.Client) from 7.6.0
to 7.9.0.

<details>
<summary>Release notes</summary>

_Sourced from [NuGet.Common's
releases](https://github.com/NuGet/NuGet.Client/releases)._

No release notes found for this version range.

Commits viewable in [compare
view](https://github.com/NuGet/NuGet.Client/commits).
</details>

Updated [NuGet.Packaging](https://github.com/NuGet/NuGet.Client) from
7.6.0 to 7.9.0.

<details>
<summary>Release notes</summary>

_Sourced from [NuGet.Packaging's
releases](https://github.com/NuGet/NuGet.Client/releases)._

No release notes found for this version range.

Commits viewable in [compare
view](https://github.com/NuGet/NuGet.Client/commits).
</details>

Updated [NuGet.Protocol](https://github.com/NuGet/NuGet.Client) from
7.6.0 to 7.9.0.

<details>
<summary>Release notes</summary>

_Sourced from [NuGet.Protocol's
releases](https://github.com/NuGet/NuGet.Client/releases)._

No release notes found for this version range.

Commits viewable in [compare
view](https://github.com/NuGet/NuGet.Client/commits).
</details>

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore <dependency name> major version` will close this
group update PR and stop Dependabot creating any more for the specific
dependency's major version (unless you unignore this specific
dependency's major version or upgrade to it yourself)
- `@dependabot ignore <dependency name> minor version` will close this
group update PR and stop Dependabot creating any more for the specific
dependency's minor version (unless you unignore this specific
dependency's minor version or upgrade to it yourself)
- `@dependabot ignore <dependency name>` will close this group update PR
and stop Dependabot creating any more for the specific dependency
(unless you unignore this specific dependency or upgrade to it yourself)
- `@dependabot unignore <dependency name>` will remove all of the ignore
conditions of the specified dependency
- `@dependabot unignore <dependency name> <ignore condition>` will
remove the ignore condition of the specified dependency and ignore
conditions


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Argument matchers fail if null passed in to an out parameter of a generic derived type

2 participants