Sitelet https://web.archive.org/web/20260604161143/https://github.com/github/codeql/pull/12058
Skip to content

C# 11: Check that we get AST for structs that doesn't initialise all fields.#12058

Merged
michaelnebel merged 1 commit into
github:mainfrom
michaelnebel:csharp/structdefaults
Feb 9, 2023
Merged

C# 11: Check that we get AST for structs that doesn't initialise all fields.#12058
michaelnebel merged 1 commit into
github:mainfrom
michaelnebel:csharp/structdefaults

Conversation

@michaelnebel
Copy link
Copy Markdown
Contributor

This is a test only PR.
In C# 11 it is allowed to declare structs that doesn't initialise all fields/properties.

@github-actions github-actions Bot added the C# label Feb 1, 2023
@michaelnebel michaelnebel force-pushed the csharp/structdefaults branch from 7136f6c to ae10a6b Compare February 2, 2023 11:53
@michaelnebel michaelnebel changed the title C# 11: Check that we get AST for struct that doesn't initialize all fields. C# 11: Check that we get AST for structs that doesn't initialise all fields. Feb 2, 2023
@michaelnebel michaelnebel marked this pull request as ready for review February 2, 2023 11:53
@michaelnebel michaelnebel requested a review from a team as a code owner February 2, 2023 11:53
@michaelnebel michaelnebel requested review from mbg and tamasvajk February 2, 2023 11:53
Copy link
Copy Markdown
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

In IL, the non-initialized fields/auto-properties are explicitly assigned their default value. Do we need to synthesize these implicit assignments in QL?

@michaelnebel
Copy link
Copy Markdown
Contributor Author

In IL, the non-initialized fields/auto-properties are explicitly assigned their default value. Do we need to synthesize these implicit assignments in QL?

Good question.
This will require doing some definite assignment analysis, which has to be equivalent to that of the C# compiler itself.
Do you know of any other examples where we do this?

@tamasvajk
Copy link
Copy Markdown
Contributor

Good question. This will require doing some definite assignment analysis, which has to be equivalent to that of the C# compiler itself. Do you know of any other examples where we do this?

I don't know. I found two PRs that are somewhat related:

@michaelnebel
Copy link
Copy Markdown
Contributor Author

cc @hvitved : What do you think?

@michaelnebel michaelnebel requested a review from hvitved February 7, 2023 10:06
@hvitved
Copy link
Copy Markdown
Contributor

hvitved commented Feb 8, 2023

If we were to synthesize default assignments, that would have to be done in the extractor.

However, we also don't generate default assignments for class fields, so I don't think we want to do it for structs.

@michaelnebel
Copy link
Copy Markdown
Contributor Author

If we were to synthesize default assignments, that would have to be done in the extractor.

However, we also don't generate default assignments for class fields, so I don't think we want to do it for structs.

The difference is that with C# 11 it is possible to not explicitly initialize all fields of a struct. If we decides to not assign a value then the compiler will explicitly insert an instruction to do so.
That is for classes,

public class C {
    private readonly int A;
    private readonly int B;
    
    public C() { 
        A = 0;
    }
}

Compiling and de-compiling is the identify, but for following declaration

public struct S {
    private readonly int A;
    private readonly int B;
    
    public S() { 
        A = 0;
    }
}

compiling and de-compiling yields

public struct S
{
    private readonly int A;

    private readonly int B;

    public S()
    {
        B = 0;
        A = 0;
    }
}

@hvitved
Copy link
Copy Markdown
Contributor

hvitved commented Feb 9, 2023

The difference is that with C# 11 it is possible to not explicitly initialize all fields of a struct. If we decides to not assign a value then the compiler will explicitly insert an instruction to do so.

Still, fields of classes also get a default value (i.e., the field B of the class C in your example will have value 0), but we don't synthesize the implicit assignment

private readonly int B = default(int);

@michaelnebel
Copy link
Copy Markdown
Contributor Author

The difference is that with C# 11 it is possible to not explicitly initialize all fields of a struct. If we decides to not assign a value then the compiler will explicitly insert an instruction to do so.

Still, fields of classes also get a default value (i.e., the field B of the class C in your example will have value 0), but we don't synthesize the implicit assignment

private readonly int B = default(int);

I am convinced :-)
If you approve the PR, I will merge.

@michaelnebel michaelnebel merged commit b895065 into github:main Feb 9, 2023
@michaelnebel michaelnebel deleted the csharp/structdefaults branch February 9, 2023 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants