Skip to content

Conversation

@ThomasGoulet73
Copy link
Contributor

Description

Update LangVersion to C# 9. WPF seems to be the only repo that still use C# 8.

Here's a list of popular dotnet repos and their version:
aspnetcore: 9.0
efcore: 9.0
runtime: preview
winforms: preview

This change allows us to use newer feature (E.g. Static anonymous functions: #4717 (comment))

Customer Impact

None

Regression

No

Testing

I tested by doing a build locally.

Risk

There shouldn't be any risk, C# 9 is already the default version for .Net 6.

@ThomasGoulet73 ThomasGoulet73 requested a review from a team as a code owner June 24, 2021 23:04
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 24, 2021
@ghost ghost requested review from SamBent, fabiant3 and ryalanms June 24, 2021 23:04
@RussKie
Copy link
Contributor

RussKie commented Jul 1, 2021

Why not preview?

@vatsan-madhavan
Copy link
Member

Has WPF adopted C# 9's module initializer support yet for PresentationCore, and stopped using ILDAsm/ILAsm round-tripping ? If not, then do that first before doing this here.

@ThomasGoulet73
Copy link
Contributor Author

@RussKie, I didn't update to preview or latest because they might change depending on the .Net version and I assumed the version was pinned in the first place because of this. Doing this might require a bigger discussion than this PR requires, which just changes the pinned version to be the default version of .Net 6. I didn't want to extend the already "slow" PR acceptance delay.

@vatsan-madhavan As far as I know, WPF didn't migrate to C# 9 module initializer and the issue about this is sill opened (#3885). I'm not sure how this would block this PR though. Maybe you were talking about @RussKie's comment ?

@vatsan-madhavan
Copy link
Member

ILDasm-ILAsm roundtripping, from what I'd heard at that time, doesn't always keep pace with language improvements. I chose to fix the LangVersion property to a known constant that was known to work well for PresentationCore - instead of preview or latest - so that ILDasm/ILAsm roundtripping of PresentationCore would operate on a well-understood set of language constructs. I especially didn't want PresentationCore to end up in a situation in which the roundtripping seemed to happen without problems (i.e., sans build/CI failures), but introduced subtle errors.

If this situation has improved - i.e., if ILDasm/ILAsm round-tripping has become more robust over the past ~year, then changing LangVersion to preview or latest ought to be ok. You can also do things more piecemeal - like increasing LangVersion globally and then setting it to 8 for PresentationCore but that sounds rather kludgy.

That said, #3885 is not a hard problem; I view it as an ~1 day effort item.

/cc @rladuca, @danmoseley

@RussKie
Copy link
Contributor

RussKie commented Jul 9, 2021

We need want to update to C#10 for a number of reasons.

  1. In .NET 6 GA (or really from Preview 6 and on) targeting net6.0 in an application will also target C# 10 by default. That will enable all C# 10 features including global usings, file scoped namespaces, etc...
  2. To enable global usings support in WPF (WPF project templates use latest C# language idioms #4774)

@lindexi
Copy link
Member

lindexi commented Jul 21, 2021

See #4889

@ryalanms
Copy link
Member

@vatsan-madhavan and @SamBent: I've replaced the IL injection with System.Runtime.CompilerServices.ModuleInitializerAttribute, but diff'ing the IL from each shows significant differences beyond the module initializer. My expectation was that the IL injection would only modify what is necessary to simulate the module initializer, yet there are all sorts of minor changes unrelated to the initializer in the IL.

Anyway, the module initializer without IL injection is working as expected:

PresentationCore.dll!ModuleInitializer.Initialize() Line 21 C#
PresentationCore.dll!.() Unknown
[Native to Managed Transition]

@rladuca
Copy link
Member

rladuca commented Jul 23, 2021

@vatsan-madhavan and @SamBent: I've replaced the IL injection with System.Runtime.CompilerServices.ModuleInitializerAttribute, but diff'ing the IL from each shows significant differences beyond the module initializer. My expectation was that the IL injection would only modify what is necessary to simulate the module initializer, yet there are all sorts of minor changes unrelated to the initializer in the IL.

Anyway, the module initializer without IL injection is working as expected:

PresentationCore.dll!ModuleInitializer.Initialize() Line 21 C#
PresentationCore.dll!.() Unknown
[Native to Managed Transition]

I think what might be happening is that we were round-tripping IL. The tasks would disassemble the entire PresentationCore binary, inject the initialize, then reassemble it. So there are going to be many small round-tripping artifacts. I'm not sure of the differences you are seeing, but I would imagine these issues would account for them. With your, much cleaner, method you should now see "pristine" IL right from the compiler.

@ryalanms
Copy link
Member

Thanks, everyone, for the guidance.

@ryalanms ryalanms merged commit 9d4a6f9 into dotnet:main Jul 23, 2021
@ThomasGoulet73 ThomasGoulet73 deleted the csharp9 branch July 29, 2021 18:42
@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants