[main] Source code updates from dotnet/dotnet - #131569
Conversation
Diff: https://github.com/dotnet/dotnet/compare/3dc10d7035d97e4a50afc9a1be854b9c43f5e9a1..813f634ceb016018f5acc9bd3c2b16e17dff4686 From: dotnet/dotnet@3dc10d7 To: dotnet/dotnet@813f634 [[ commit created by automation ]]
Updated Dependencies: Microsoft.CodeAnalysis, Microsoft.CodeAnalysis.Analyzers, Microsoft.CodeAnalysis.BannedApiAnalyzers, Microsoft.CodeAnalysis.CSharp, Microsoft.Net.Compilers.Toolset (Version 5.10.0-1.26365.101 -> 5.10.0-1.26379.102) Microsoft.CodeAnalysis.NetAnalyzers, Microsoft.DotNet.ApiCompat.Task, Microsoft.NET.Workload.Emscripten.Current.Manifest-11.0.100.Transport (Version 11.0.100-preview.7.26365.101 -> 11.0.100-rc.1.26379.102) Microsoft.DotNet.Arcade.Sdk, Microsoft.DotNet.Build.Tasks.Archives, Microsoft.DotNet.Build.Tasks.Feed, Microsoft.DotNet.Build.Tasks.Installers, Microsoft.DotNet.Build.Tasks.Packaging, Microsoft.DotNet.Build.Tasks.TargetFramework, Microsoft.DotNet.Build.Tasks.Templating, Microsoft.DotNet.CodeAnalysis, Microsoft.DotNet.GenAPI, Microsoft.DotNet.GenFacades, Microsoft.DotNet.Helix.Sdk, Microsoft.DotNet.PackageTesting, Microsoft.DotNet.RemoteExecutor, Microsoft.DotNet.SharedFramework.Sdk, Microsoft.DotNet.XliffTasks, Microsoft.DotNet.XUnitExtensions (Version 11.0.0-beta.26365.101 -> 11.0.0-beta.26379.102) runtime.linux-arm64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.linux-x64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.linux-musl-x64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.win-arm64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.win-x64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.osx-arm64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.osx-x64.Microsoft.NETCore.Runtime.JIT.Tools, runtime.linux-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Libclang, runtime.linux-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk, runtime.linux-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools, runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Libclang, runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk, runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools, runtime.linux-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Libclang, runtime.linux-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk, runtime.linux-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools, runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Libclang, runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk, runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools, runtime.win-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Libclang, runtime.win-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk, runtime.win-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools, runtime.osx-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Libclang, runtime.osx-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk, runtime.osx-arm64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools, runtime.osx-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Libclang, runtime.osx-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Sdk, runtime.osx-x64.Microsoft.NETCore.Runtime.Mono.LLVM.Tools (Version 23.1.0-alpha.1.26357.1 -> 23.1.0-alpha.1.26370.1) Microsoft.DotNet.Cecil (Version 0.11.5-preview.26365.101 -> 0.11.5-preview.26379.102) Microsoft.DotNet.XUnitConsoleRunner (Version 2.9.3-beta.26365.101 -> 2.9.3-beta.26379.102) Microsoft.NET.Sdk.IL, Microsoft.NETCore.App.Ref, Microsoft.NETCore.ILAsm, runtime.native.System.IO.Ports, System.Reflection.Metadata, System.Reflection.MetadataLoadContext, System.Text.Json (Version 11.0.0-preview.7.26365.101 -> 11.0.0-rc.1.26379.102) NuGet.Frameworks, NuGet.Packaging, NuGet.ProjectModel, NuGet.Versioning (Version 7.10.0-rc.36601 -> 7.10.0-rc.38002) System.CommandLine (Version 3.0.0-preview.7.26365.101 -> 3.0.0-rc.1.26379.102) runtime.linux-arm64.Microsoft.NETCore.Runtime.Wasm.Node.Transport, runtime.linux-musl-arm64.Microsoft.NETCore.Runtime.Wasm.Node.Transport, runtime.linux-x64.Microsoft.NETCore.Runtime.Wasm.Node.Transport, runtime.linux-musl-x64.Microsoft.NETCore.Runtime.Wasm.Node.Transport, runtime.osx-arm64.Microsoft.NETCore.Runtime.Wasm.Node.Transport, runtime.osx-x64.Microsoft.NETCore.Runtime.Wasm.Node.Transport, runtime.win-arm64.Microsoft.NETCore.Runtime.Wasm.Node.Transport, runtime.win-x64.Microsoft.NETCore.Runtime.Wasm.Node.Transport (Version 11.0.0-alpha.1.26353.1 -> 11.0.0-alpha.1.26372.1) Added Dependencies: Microsoft.DotNet.Build.Tasks.FileCatalog (Version 11.0.0-beta.26379.102) Removed Dependencies: Microsoft.DotNet.Build.Tasks.Workloads (Version 11.0.0-beta.26365.101) [[ commit created by automation ]]
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
…rences The new Roslyn toolset compiles `_ = ref x;` as a value discard, emitting `ldarg; ldind.ref; pop` instead of nothing. In StructureMarshaler<T> those statements existed only to satisfy IDE0060 for the managed fallback bodies of JIT intrinsics, whose callers (the Marshal.LayoutTypeMarshalerMethods delegate path) pass `ref Unsafe.NullRef<CleanupWorkListElement?>()`. The extra dereference therefore threw NullReferenceException. Drop the discards and suppress IDE0060 with the pragma idiom already used elsewhere in this file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8591ab4e-b1d7-40df-80f0-9a476a7bfd9d
For a union case whose type is the union type itself, the emitted type pattern is trivially satisfied by the union value, so with the new Roslyn toolset the designator binds the union instead of its payload. The emitted deconstructor then returned the same value it was given and JsonUnionConverter recursed until it threw "possible object cycle". Emit an arm that reads the payload off the union's Value property instead. The parser already requires a public `object Value` property for every source-generated union. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8591ab4e-b1d7-40df-80f0-9a476a7bfd9d
Root cause analysis of the test failures in this codeflowBoth failure clusters in this PR come from the toolset bump ( The toolset delta is VMR 1.
|
|
Important While this PR was open, the source repository has received code changes from this repository (an opposite codeflow merged). You can continue with one of the following options:
💡 You may consult the FAQ for more information or tag @dotnet/prodconsvcs for assistance. |
|
@eiriktsarpalis would you mind taking a look at the STJ source generator change? thanks |
|
Also cc @jkotas for the
|
Yes, this is a breaking behavior change in Roslyn. Even though C# language model assumes that refs are never null, null refs do exist in practice even in safe code and they need to handled in predictable deterministic way by Roslyn. |
The recent change to union pattern matching appears to have regressed handling of recursive union types. Minimal repro: using System;
public union Nat(bool, Nat);
public static class Program
{
private static int Depth(Nat n) => n switch
{
bool => 0,
Nat succ => 1 + Depth(succ),
};
private static void Main()
{
Nat three = new Nat(new Nat(new Nat(true)));
Console.WriteLine(Depth(three)); // expected 2, actual stack overflow
}
}cc @dotnet/roslyn |
This is an intentional and expected change in behavior. See https://github.com/dotnet/csharplang/blob/main/proposals/unions.md#type-pattern:
|
So you're saying the only safe way to pattern match against a recursive union is to explicitly match against its
|
I think there is nothing unsafe about the pattern. You just need to understand when does it match and act accordingly when it does. If you intend to use a type pattern that is going to match the instance itself, but you don't want to match against the instance itself, then yes, using property pattern explicitly
A property pattern form against a union instance as an input value should still take advantage of non-boxing access members. Compiler can see that this is a union "unwrapping" during matching.
This is an infinite recursion through a function call, not some kind of pattern matching operation that never ends. There are many other ways to get into an infinite recursion. For example, it doesn't have to be direct, i.e. it might go through a sequence of repeated calls. Compiler almost never performs infinite recursion analysis for functions. If I remember correctly, the only exception -
Sometimes they are useful. As with any tool, they should be used properly. |
I think this is primarily what I take issue with. Pattern matching works in a very specific way for most types except for when a case type happens to be a subtype of the union itself. I expect most casual users will get tripped up by this and for everybody else it's just another concern they will have to keep in mind.
Presumably that would necessitate using
Sure, but this is the canonical pattern one would expect a recursive union to fall into. If it's not recursion it would be an infinite loop instead. |
I agree that in this particular scenario a dereference should not occur. I think this is another fall out from an alignment of handling between parameters and locals. A ref local is dereferenced in a similar situation. I opened an issue specifically for |
You can certainly raise a concern. Perhaps, https://github.com/dotnet/csharplang or a "C# Union stakeholders sync" chat would be a better place for that, since this becomes a design discussion.
That is correct. If code explicitly reads the value, a property getter will be called. |
This comment was marked as outdated.
This comment was marked as outdated.
2 similar comments
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Use `{ Value: T x }` for the recursive union case instead of a bare type
pattern. The bare pattern is trivially satisfied by the union instance, so
it never verified that the payload actually had the case type and it
shadowed every arm emitted after it. Case order follows declaration order
for unrelated case types, so a union declaring its recursive case first
produced generated code that failed to compile with CS8510.
Add a union that declares the recursive case first as a regression test.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8591ab4e-b1d7-40df-80f0-9a476a7bfd9d
|
@jkotas @eiriktsarpalis are you ok with merging the codeflow or do you think there is something blocking? |
Yes, we should get this merged and resolve the outstanding issues in a follow up |
|
Important While this PR was open, the source repository has received code changes from this repository (an opposite codeflow merged). You can continue with one of the following options:
💡 You may consult the FAQ for more information or tag @dotnet/prodconsvcs for assistance. |
|
Failure is unrelated (#131665). BA has it under both known and new failures. |
|
/ba-g unrelated issue that BA is not catching properly |
5 participants
Note
This is a codeflow update. It may contain both source code changes from
the VMR
as well as dependency updates. Learn more here.
This pull request brings the following source code changes
From https://github.com/dotnet/dotnet
New Dependencies
Removed Dependencies
Updated Dependencies
Associated changes in source repos
Diff the source with this PR branch