Skip to content

Fix null reference crash in CheckUnderspecifiedGenericExtension for namespace-level extensions#80929

Merged
jcouv merged 5 commits intomainfrom
copilot/fix-check-underspecified-generic-extension
Nov 19, 2025
Merged

Fix null reference crash in CheckUnderspecifiedGenericExtension for namespace-level extensions#80929
jcouv merged 5 commits intomainfrom
copilot/fix-check-underspecified-generic-extension

Conversation

Copy link
Contributor

Copilot AI commented Oct 28, 2025

Fixes #80829

…ce-level extensions

Co-authored-by: jcouv <12466233+jcouv@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix crash in CheckUnderspecifiedGenericExtension for namespace extensions Fix null reference crash in CheckUnderspecifiedGenericExtension for namespace-level extensions Oct 28, 2025
Copilot AI requested a review from jcouv October 28, 2025 02:05
@jcouv jcouv added Area-Compilers Feature - Extension Everything The extension everything feature labels Nov 16, 2025
@jcouv jcouv marked this pull request as ready for review November 16, 2025 14:33
@jcouv jcouv requested a review from a team as a code owner November 16, 2025 14:33

NamedTypeSymbol extension = extensionMember.ContainingType;
if (extension.ExtensionParameter is not { } extensionParameter || extension.ContainingType.Arity != 0)
if (extension.ExtensionParameter is not { } extensionParameter || extension.ContainingType?.Arity != 0)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContainingType

I think we should check for other places in code where we unconditionally accessing ContainingType on an extension type. #Closed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked usages of ContainingType in context of usages of GetIsNewExtensionMember, ExtensionParameter and IsExtension.
I didn't find any problem, but added a couple of tests to ensure the null checks are covered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked usages of ContainingType in context of usages of GetIsNewExtensionMember, ExtensionParameter and IsExtension. I didn't find any problem, but added a couple of tests to ensure the null checks are covered.

I thought GetIsNewExtensionMember got renamed. Did you use new name for the check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR was forked before the rename. I refreshed the PR and took another look (now for IsExtensionBlockMember()). Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, for other APIs, I don't think it's worthwhile to do another pass. We didn't change extension logic that much in the last 3 weeks.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 17, 2025

Done with review pass (commit 3) #Closed

@jcouv jcouv requested review from AlekseyTs and jjonescz November 18, 2025 08:55
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 4)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 5)

@jcouv jcouv merged commit 59b64d7 into main Nov 19, 2025
25 checks passed
@jcouv jcouv deleted the copilot/fix-check-underspecified-generic-extension branch November 19, 2025 20:11
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 19, 2025
@jcouv
Copy link
Member

jcouv commented Dec 9, 2025

/backport to release/dev18.0

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2025

Started backporting to release/dev18.0 (link to workflow run)

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2025

@jcouv backporting to release/dev18.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Creating an empty commit: Initial plan
Applying: Fix null reference in CheckUnderspecifiedGenericExtension for namespace-level extensions
Applying: Fixup test
Applying: Address feedback
error: sha1 information is lacking or useless (src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0004 Address feedback
Error: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

jcouv pushed a commit to jcouv/roslyn that referenced this pull request Dec 9, 2025
jcouv added a commit that referenced this pull request Dec 10, 2025
…ricExtension for namespace-level extensions (#81613)

Fixes #80829

Cherry-picked with minor modification from #80929
@davidwengier davidwengier modified the milestones: Next, 18.3 Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Extension Everything The extension everything feature

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

It looks like CheckUnderspecifiedGenericExtension is going to crash for an extension block declared in a namespace

5 participants