Conversation
2a012c0 to
7ce18b7
Compare
|
This is still not ready to merge (some sloppy copy-pasting and incomplete checks around 'extern static' constructors). But I would like to open it for input. I am unsure whether to skip the static constructor based on the lowered bound tree, or whether to skip it based on the emitted body containing only
/cc @AlekseyTs. |
|
We should never eliminate an explicitly declared constructor. Also, for the first phase we can simply handle the scenario where bound initializers (after initial binding) produce obviously "default" values, i.e. no conditional logic, etc. #Closed |
|
I misread #42985 (comment) to mean that we could even eliminate explicitly declared |
jaredpar
left a comment
There was a problem hiding this comment.
Curious: why can’t we eliminate static constructors when they are just setting default values? Thought for a bit and can’t see that being a compat break. Is there one I’m missing or a completely different issue at play here?
|
The first thing that comes to mind for me is a user may expect to be able to add an empty static constructor and set a breakpoint in it just to debug something about their program’s initialization. Eliminating the explicitly declared but empty static constructors is something we could conceivably do, but it is riskier and less compelling. We probably wouldn’t be making this change for the synthesized static constructors if not for the |
That makes sense. If a user explicitly adds one then we should not be deleting it. I was thinking more of the cases where we implicitly generate one for them. |
| if (methodSymbol is SynthesizedStaticConstructor) | ||
| { | ||
| foreach (var initializer in initializers) | ||
| { |
There was a problem hiding this comment.
{ [](start = 24, length = 1)
Consider extracting the loop body to a helper method and use return initializers.Any(i => HasStatements(i));.
| // Although we do not emit the synthesized static constructor, the source type symbol will still appear to have one | ||
| Assert.True(HasSynthesizedStaticConstructor(typeSymbol)); | ||
| Assert.True(IsBeforeFieldInit(typeSymbol)); | ||
| } |
There was a problem hiding this comment.
} [](start = 8, length = 1)
Please test:
static S s1 = default;
static S? s2 = null;
static S? s3 = default;
static S? s4 = default(S);
static object s5 = default(S);
There was a problem hiding this comment.
Consider using a Theory to keep the test compact.
src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs
Outdated
Show resolved
Hide resolved
Was this addressed? In reply to: 615538957 [](ancestors = 615538957) |
There was no response to this question. In reply to: 615535416 [](ancestors = 615535416) Refers to: src/Compilers/CSharp/Test/Emit/PDB/PDBAsyncTests.cs:37 in bd74323. [](commit_id = bd74323, deletion_comment = False) |
|
Done with review pass (iteration 15) #Closed |
Modified
Added some tests specifically to verify the .cctor that gets created for a const decimal field and for a non-capturing lambda. |
|
It also sounds like I should also start using symbolValidator to check whether or not a .cctor is present. Will make a pass to fix that when I get the chance. |
| var source = @" | ||
| class C | ||
| { | ||
| const decimal d1 = 0.1m; |
There was a problem hiding this comment.
0.1m [](start = 23, length = 4)
Let's also add a test with 0m. Will it be treated as default and eliminate the constructor? #Closed
There was a problem hiding this comment.
I found that 0.0m was treated differently than 0 or default. That seems like a bug.
There was a problem hiding this comment.
(actually, it seems like the precision of 0.0m made it a non-default value. 0m is treated as expected.)
|
Done with review pass (iteration 17) #Closed |
|
Could I please get a second review @dotnet/roslyn-compiler |
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedStaticConstructor.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenConstructorInitTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenConstructorInitTests.cs
Outdated
Show resolved
Hide resolved
…-pointers * upstream/master: (119 commits) Apply FixAll for IDE0044 (Make field readonly) for newly added fields Fix empty cctor (dotnet#43234) Add tests Avoid using an ArrayBuilder for IntervalTree<T>.Any Add tests Add in tests Do not remove cast when it would break a params call. Add tests Do not remove non-identity casts in conditional expressions Don't remove null-casts in switch expressions Updte comment Update src/Analyzers/Core/Analyzers/UseSystemHashCode/UseSystemHashCodeDiagnosticAnalyzer.cs Add assert bad attempt change variable name to Add_file_header Update eng/build-utils.ps1 Use consistent namespaces for integration tests call it a header instead of a banner Improve VS Code build current project support Add docs ...
Closes #42985
Solves the issue by excluding the .cctor from the public members list and waiting until emit to decide whether to synthesize the constructor.Solves the issue without changing the conditions where we include the synthesized static constructor in GetMembers(). Instead we decide selectively to not emit the synthesized static constructor if the field initializers are only assigning the default values.
We could try to skip emitting even an explicitly declared static constructor if we find that the lowered implementation is empty. I feel that this is a bit more risky/difficult to get right.We will only eliminate synthesized static constructors.