Skip to content

Report errors for union members that are not permitted#82626

Merged
AlekseyTs merged 1 commit into
dotnet:features/Unionsfrom
AlekseyTs:Unions_26
Mar 4, 2026
Merged

Report errors for union members that are not permitted#82626
AlekseyTs merged 1 commit into
dotnet:features/Unionsfrom
AlekseyTs:Unions_26

Conversation

@AlekseyTs

Copy link
Copy Markdown
Contributor

No description provided.

@333fred 333fred left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with a test suggestion.


union S7(int, bool)
{
public S7()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider testing default parameters, both as a single arg and as a second arg.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Consider testing default parameters, both as a single arg and as a second arg.

I do not find this scenario interesting because default parameters do not change amount of parameters.

@AlekseyTs

Copy link
Copy Markdown
Contributor Author

@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler For a second review

Diagnostic(ErrorCode.ERR_UnionConstructorCallsDefaultConstructor, "S10").WithLocation(36, 5),
// (53,7): error CS9406: A constructor declared in a 'union' declaration must have a 'this' initializer that calls a synthesized constructor or an explicitly declared constructor.
// : this()
Diagnostic(ErrorCode.ERR_UnionConstructorCallsDefaultConstructor, "this").WithLocation(53, 7)

@jjonescz jjonescz Mar 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have a test for a union declaration with a constructor with a this initializer that calls an explicitly declared constructor? #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we have a test for a union declaration with a constructor with a this initializer that calls an explicitly declared constructor?

The first constructor in S9

union S9(int, bool)
{
    S9(int x, bool y)
    : this()
    {}

#line 30
    public S9()
    {}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, I guess I would also like to see a test where that called constructor has some parameters.

@AlekseyTs AlekseyTs merged commit 7f8a7b3 into dotnet:features/Unions Mar 4, 2026
24 checks passed
333fred added a commit to 333fred/roslyn that referenced this pull request Mar 9, 2026
* upstream/main: (56 commits)
  [main] Source code updates from dotnet/dotnet (dotnet#82655)
  Update insert.yml to use roslyn-tools for insertions (dotnet#82615)
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2921538
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2921538
  Reduce allocations during source text diffing (dotnet#82462)
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2920904
  Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2920904
  Revert "Throw NotImplementedException when a build errorId is not supported" (dotnet#82638)
  [main] Source code updates from dotnet/dotnet (dotnet#82623)
  Revert System.ValueTuple version update (dotnet#82639)
  Pack enum values
  Track non-blocking PROTOTYPE comments for `Unions` feature in an issue (dotnet#82637)
  Update the build system errors instructions for Visual Studio 2026
  Don't log named pipe connection failures as errors in BuildServerConnection (dotnet#82609)
  Fix resx source generator for less common resx metadata and locations (dotnet#81994)
  Disallow `ref` modifier for a union declaration (dotnet#82632)
  Add IFSharpInlineHintsService2 for displayAllOverride support (dotnet#82629)
  Report errors for union members that are not permitted (dotnet#82626)
  Check language version for union matching and conversions (dotnet#82617)
  [main] Update dependencies from dotnet/arcade (dotnet#82618)
  ...
Comment on lines +24221 to +24222
S1(string x)
: this(1) {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This constructor is private, is it really expected to be reported? Message says only about "public constructors" 🤔

@AlekseyTs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This constructor is private, is it really expected to be reported? Message says only about "public constructors" 🤔

Good catch. Looks like an oversight.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Insane speed by the way :)

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.

4 participants