Implement support for captured Primary Constructor parameters in EE when portable PDB is used. #67266
Conversation
…hen portable PDB is used. Closes dotnet#67107
|
@dotnet/roslyn-compiler Please review |
src/ExpressionEvaluator/Core/Source/ExpressionCompiler/PDB/MethodDebugInfo.Portable.cs
Outdated
Show resolved
Hide resolved
| @@ -309,9 +315,10 @@ internal static (Guid ModuleVersionId, ISymUnmanagedReader SymReader, int Method | |||
| string expr, | |||
| int atLineNumber = -1, | |||
| bool includeSymbols = true, | |||
| TargetFramework targetFramework = TargetFramework.Standard) | |||
| TargetFramework targetFramework = TargetFramework.Standard, | |||
| DebugInformationFormat debugFormat = DebugInformationFormat.Pdb) | |||
There was a problem hiding this comment.
Is this changing the default here so now even existing tests are evaluated only against PDB and not also against portable PDB as previously? #Closed
There was a problem hiding this comment.
Is this changing the default here so now even existing tests are evaluated only against PDB and not also against portable PDB as previously?
That was not my intent and I do not think the default is changed. See the change on the line 369 below.
| resultProperties: out _, | ||
| error: out string error); | ||
|
|
||
| Assert.Equal("error CS0103: The name 'y' does not exist in the current context", error); |
There was a problem hiding this comment.
Why can't we access y here? It's just a field like x, right? #Closed
There was a problem hiding this comment.
Why can't we access y here? It's just a field like x, right?
The lambda doesn't have access to the instance because this isn't captured. <>c__DisplayClass0_0 has only x.
internal class C
{
[CompilerGenerated]
private sealed class <>c__DisplayClass0_0
{
public int x;
internal int <.ctor>b__0()
{
return x;
}
}
[CompilerGenerated]
private int <y>PC__BackingField;
[System.Runtime.CompilerServices.Nullable(1)]
private Func<int> Y;
public C(int y, int x)
{
<y>PC__BackingField = y;
<>c__DisplayClass0_0 <>c__DisplayClass0_ = new <>c__DisplayClass0_0();
<>c__DisplayClass0_.x = x;
Y = new Func<int>(<>c__DisplayClass0_.<.ctor>b__0);
base..ctor();
}
private int M()
{
return <y>PC__BackingField;
}
}
|
@cston, @dotnet/roslyn-compiler For the second review |
| MethodSymbol currentFrame, | ||
| ImmutableArray<string> shadowingParameterNames, | ||
| TypeSymbol possiblyCapturingType, | ||
| (DisplayClassInstance? Instance, ConsList<FieldSymbol> Fields) possiblyCapturingTypeInstance, |
There was a problem hiding this comment.
Why a tuple rather than two separate parameters?
The values are closely related and I felt the code is easier to follow this way. At least it was easier to write for me.
| sawCapturedParameters = true; | ||
|
|
||
| if (!displayClassVariablesBuilder.ContainsKey(parameterName) && | ||
| !shadowingParameterNames.Any((n1, n2) => n1 == n2, parameterName)) |
| displayClassVariablesBuilder, displayClassVariableNamesOutsideInOrderBuilder); | ||
| } | ||
|
|
||
| if (checkForPrimaryConstructor && displayClassVariablesBuilder.Values.FirstOrDefault(v => v.Kind == DisplayClassVariableKind.This) is { } thisProxy) |
There was a problem hiding this comment.
Should the check include
&& !currentFrame.IsStatic?
I don't think we care if the method itself is static as long as it has a "thisProxy" which can be supplied trough a parameter.
There was a problem hiding this comment.
All we care is whether we have access to this by whatever means
| } | ||
|
|
||
| [Fact] | ||
| public void PrimaryConstructors_01201_CapturedParameterInsideLocalFunctionInInstanceMethod_WithDisplayClass() |
There was a problem hiding this comment.
How does this test differ from the previous test? Consider adding a comment.
The difference is in the name of the primary constructor parameter - "value". See the same name used for display class parameter of the frame method - System.Int32 <>x.<>m0(C <>4__this, ref C.<>c__DisplayClass2_0 value)
src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/CompilationContext.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
ghost
added this to the 
Closes #67107
Closes #67103