Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 17, 2025

Fixes #48243

  • Understand the issue: Need to improve error message for record inheritance without parameter list
  • Add new error code ERR_UnexpectedArgumentListInBaseTypeWithoutParameterList (CS9339)
  • Update error message resource file with helpful message
  • Update code to use the new error when appropriate
  • Update .xlf translation files
  • Add tests to verify the new error message
  • Update all existing tests to expect new error message
  • All tests passing (713 RecordTests + PrimaryConstructorTests)
  • Code review completed with no issues
  • Address PR feedback: Add WorkItem attributes, use raw string literals, fix formatting
  • Fix compiler error in ErrorFacts.cs switch statement
  • Merge [Fact] and [WorkItem] attributes on single line
  • Add coverage tests for TypeKind.Class vs struct and TypeKind.Interface scenarios
  • Remove blank line from SourceNamedTypeSymbol_Bases.cs
  • Add guideline to CONTRIBUTING.md about not using blank lines
  • Remove all trailing spaces from SourceNamedTypeSymbol_Bases.cs
  • Add guideline to copilot instructions about avoiding trailing spaces and blank lines
  • Fix formatting in PrimaryConstructorTests.cs (move closing parens to same line)
  • Fix LanguageVersion_02 and LanguageVersion_06 tests to handle both class-to-class and struct-to-interface scenarios correctly

Summary

Successfully improved the compiler error message for records and classes that attempt to pass arguments to their base type without having a parameter list.

Before:

error CS8861: Unexpected argument list.

After:

error CS9339: Cannot pass arguments to the base type without a parameter list on the type declaration. Consider adding an empty parameter list to 'R2'.

Example:

Problem code:

record R1(int P1);
record R2 : R1(1);  // Error!

Fix:

record R1(int P1);
record R2() : R1(1);  // Correct - added empty parameter list

The new error message provides clear guidance on what's wrong and how to fix it.

PR Feedback Addressed

  • Added WorkItem attributes to all new tests (references issue Wrong behavior when using records with primary constructors and inheritance? #48243)
  • Converted test strings to raw string literals (C# 11+ feature)
  • Fixed formatting to not place final close paren on new line
  • Added new error code to ErrorFacts.cs switch to fix compiler warning
  • Merged [Fact] and [WorkItem] attributes on single line
  • Added tests for struct and interface scenarios to validate the TypeKind checks
  • Removed blank line (line with only whitespace) from SourceNamedTypeSymbol_Bases.cs
  • Added coding guideline to CONTRIBUTING.md emphasizing that lines should not contain only whitespace
  • Removed all trailing spaces from SourceNamedTypeSymbol_Bases.cs
  • Added guideline to .github/copilot-instructions.md stating "Avoid trailing spaces and blank lines (lines with only whitespace). Ensure all lines either have content or are completely empty."
  • Fixed formatting in PrimaryConstructorTests.cs per code review suggestions (3 locations)
  • Fixed LanguageVersion_02 and LanguageVersion_06 tests to correctly expect ERR_UnexpectedArgumentListInBaseTypeWithoutParameterList for class-to-class inheritance and ERR_UnexpectedArgumentList for other type combinations

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 4 commits October 17, 2025 12:14
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix behavior of records with primary constructors and inheritance Improve error message for record inheritance without parameter list Oct 17, 2025
Copilot AI requested a review from CyrusNajmabadi October 17, 2025 12:43
Copilot finished work on behalf of CyrusNajmabadi October 17, 2025 12:43
);
}

[Fact]
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 17, 2025

Choose a reason for hiding this comment

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

@copilot add WorkItem attributes to all the tests you added. Use raw string literals for all test cases. Do not place the final close paren of the Verify Diagnostics call on a new line. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

@copilot update the switch in src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs to fix the compiler error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added WorkItem attributes, converted to raw string literals, fixed formatting, and updated ErrorFacts.cs switch in commit a8c6b5a.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Updated the switch in ErrorFacts.cs to include ERR_UnexpectedArgumentListInBaseTypeWithoutParameterList in commit a8c6b5a.

…, fix formatting, and update ErrorFacts switch

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Copilot AI requested a review from CyrusNajmabadi October 17, 2025 13:05
Copilot finished work on behalf of CyrusNajmabadi October 17, 2025 13:05
Diagnostic(ErrorCode.ERR_UnexpectedArgumentListInBaseTypeWithoutParameterList, "(1)").WithArguments("R2").WithLocation(2, 15));
}

[Fact]
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 17, 2025

Choose a reason for hiding this comment

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

@copilot please merge the [Fact, WorkItem(...)] pieces.

Also, please add tests that validate coverage for the checks you included for TypeKind.Class and TypeKind.Interface #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Merged attributes to single line format [Fact, WorkItem(...)] and added three tests for coverage: StructWithArgumentListInBaseType (validates TypeKind != Class), RecordStructWithArgumentListInBaseType (validates record struct), and RecordWithArgumentListToInterface (validates baseType.TypeKind == Interface). All tests pass. Commit 3e8f2a1.

… interface scenarios

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Copilot AI requested a review from CyrusNajmabadi October 17, 2025 13:14
Copilot finished work on behalf of CyrusNajmabadi October 17, 2025 13:14
{
diagnostics.Add(ErrorCode.ERR_UnexpectedArgumentList, primaryConstructorBaseType.ArgumentList.Location);
var typeDecl = (TypeDeclarationSyntax)decl.SyntaxReference.GetSyntax();

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 17, 2025

Choose a reason for hiding this comment

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

@copilot lines cannot be blank. Fix this line, and add a note to the contributing guidelines emphasizing this. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Removed the blank line from SourceNamedTypeSymbol_Bases.cs line 644 and added a guideline to CONTRIBUTING.md stating "DO NOT use blank lines (lines with only whitespace). Ensure all lines either have content or are completely empty." Commit 8f9d4c2.

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Copilot AI requested a review from CyrusNajmabadi October 17, 2025 14:13
Copilot finished work on behalf of CyrusNajmabadi October 17, 2025 14:13
var typeDecl = (TypeDeclarationSyntax)decl.SyntaxReference.GetSyntax();

// Check if this is a class inheriting from a class but missing a parameter list
if (TypeKind == TypeKind.Class &&
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Oct 17, 2025

Choose a reason for hiding this comment

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

@copilot this line ends with a trailing space (as do several others). Fix this to not have the trailing space. Also update the copilot instructions to state that trailing spaces and blank lines must be avoided. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Removed all trailing spaces from SourceNamedTypeSymbol_Bases.cs and added guideline to .github/copilot-instructions.md under Key Conventions: "Code Formatting: Avoid trailing spaces and blank lines (lines with only whitespace). Ensure all lines either have content or are completely empty." Commit 4a7e9f3.

Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Copilot AI requested a review from CyrusNajmabadi October 17, 2025 15:15
@CyrusNajmabadi CyrusNajmabadi requested a review from jcouv November 6, 2025 19:44
if (isClassInheritingFromClass)
{
comp.VerifyDiagnostics(
// (3,5): error CS9339: Cannot pass arguments to the base type without a parameter list on the type declaration. Consider adding an empty parameter list to 'Point'.
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 7, 2025

Choose a reason for hiding this comment

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

Consider adding an empty parameter list to 'Point'.

Would this suggestion work with each of the tested language versions? #Closed

Copy link
Member

Choose a reason for hiding this comment

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

The error is now:

                    // (3,5): error CS9343: Cannot pass arguments to the base type without a parameter list on the type declaration.
                    // Base()

Which i think is reasonable, regardless of lang version. The code is def in error no matter what, but the primary error (to me) is the use of () on the base-list without a parameter list on the type. Now, if you were to add such a parameter list (removing this error), you'd have the secondary error mentioning you need the right lang version.

This flow makes sense to me and balances error messages and ordering.

// Check if this is a class inheriting from a class but missing a parameter list
if (TypeKind == TypeKind.Class &&
baseType.TypeKind != TypeKind.Interface &&
decl.SyntaxReference.GetSyntax() is TypeDeclarationSyntax { ParameterList: null })
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 7, 2025

Choose a reason for hiding this comment

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

decl.SyntaxReference.GetSyntax() is TypeDeclarationSyntax { ParameterList: null }

There is absolutely no need to rewrite this with a pattern. In fact, we want to use cast because, unlike a pattern, it doesn't fail silently. So, while working under Compilers, please, stop rewriting conditions to use patterns due to personal stylistic preference. Changes like that aren't welcome unless you intend to change behavior. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

And I mean behavior of the expression that is getting switched to a pattern

Copy link
Member

Choose a reason for hiding this comment

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

this was intentional as the code in SourceNamedTypeSymbolBase seems to indicate that it can support BaseTypeDeclarationSyntax so i didn't want to blindly cast. I couldn't prove to myself that even if TypeKind was .Class that this was a guarantee. So it made sense to me to write it this way.

If you're sure that this is an invariant, and should crash otherwise, i'm happy to change. But this was not a 'style' choice. This was intentional checking, and would have been done, even without a pattern as i thought aactually validating possible cases was appropriate given how many symbols/etc. in the compiler layer can wrap varying underlying syntax constructs.

Copy link
Contributor

@AlekseyTs AlekseyTs Nov 7, 2025

Choose a reason for hiding this comment

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

If you're sure that this is an invariant

There is absolutely no need to worry about that in context of this PR because the original conditions can be evaluated in the same order and with the same short-circuiting logic. Therefore, there is absolutely no need to change them in this PR

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 7, 2025

Done with review pass (commit 17) #Closed

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 28)

@CyrusNajmabadi CyrusNajmabadi merged commit fb3fc8a into main Nov 14, 2025
25 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Nov 14, 2025
@CyrusNajmabadi CyrusNajmabadi deleted the copilot/fix-records-inheritance-issue branch December 2, 2025 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong behavior when using records with primary constructors and inheritance?

5 participants