Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider warning for [StructLayout] usage with primary constructor #67162

Closed
jcouv opened this issue Mar 2, 2023 · 1 comment · Fixed by #67712
Closed

Consider warning for [StructLayout] usage with primary constructor #67162

jcouv opened this issue Mar 2, 2023 · 1 comment · Fixed by #67712

Comments

@jcouv
Copy link
Member

jcouv commented Mar 2, 2023

I noticed that we produce a warning if you use StructLayout on a partial (undefined ordering).
Should we do something similar for primary constructors (and record structs)?
Alternatively, we can define/specify/verify the ordering.

// (5,15): warning CS0282: There is no defined ordering between fields in multiple declarations of partial struct 'C'. To specify an ordering, all instance fields must be in the same declaration.                
Diagnostic(ErrorCode.WRN_SequentialOnPartialClass, "C").WithArguments("C")); 

This was also reported here with an example showing an unexpected execution output.

Relates to test plan #65697

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 2, 2023
@AlekseyTs AlekseyTs self-assigned this Mar 2, 2023
@jcouv jcouv added this to the C# 12.0 milestone Mar 9, 2023
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Mar 9, 2023
@jaredpar
Copy link
Member

I agree that all three of these scenarios are related because they have the following properties:

  1. The ordering of members in metadata are not specified by the language / compiler.
  2. The ordering is generally understood because the compiler is deterministic, generally it's the same as lexical order in the files, and we don't change the implementation between releases.

If we were starting from scratch I'd probably push for an error here. Given that partial set the precedence though I think we should just follow on that here and issue a warning. Do think this warning should be tied to lang version 12 though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment