Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixup partial property API usage #74951

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Fixup partial property API usage #74951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
642e019
Fixup some partial property API usage
RikkiGibson Aug 29, 2024
eea02d9
Update baseline
RikkiGibson Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.Variables.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,8 +396,6 @@ internal Variables GetRootScope()

internal Variables? GetVariablesForMethodScope(MethodSymbol method)
{
// https://github.com/dotnet/roslyn/issues/73772: is this needed if we also delete the weird cascading in EnterParameters?
method = method.PartialImplementationPart ?? method;
var variables = this;
while (true)
{
Expand Down
4 changes: 0 additions & 4 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2762,10 +2762,6 @@ private void EnterParameters()
}
}

// The partial definition part may include optional parameters whose default values we want to simulate assigning at the beginning of the method
// https://github.com/dotnet/roslyn/issues/73772: is this actually used/meaningful?
methodSymbol = methodSymbol.PartialDefinitionPart ?? methodSymbol;
Comment on lines -2765 to -2767
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and the other deleted usage of partial APIs in nullable are no longer necessary per https://github.com/dotnet/csharplang/blob/main/proposals/nullable-parameter-default-value-analysis.md#suggested-change-to-declaration-analysis.


var methodParameters = methodSymbol.Parameters;
var signatureParameters = (_useDelegateInvokeParameterTypes ? _delegateInvokeMethod! : methodSymbol).Parameters;

Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1591,6 +1591,15 @@ internal override void ForceComplete(SourceLocation? locationOpt, Predicate<Symb
}

DeclaringCompilation.SymbolDeclaredEvent(this);
if (this.IsPartialDefinition())
Copy link
Contributor Author

@RikkiGibson RikkiGibson Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This check is similar in purpose to the following check performed for ordinary methods. Essentially it seems that MethodCompiler will report the SymbolDeclaredEvents for implementation parts of partial methods (including property accessors). But for the definition parts, the symbol itself must do it.

roslyn/src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs

Lines 279 to 285 in eea02d9

protected sealed override void CompleteAsyncMethodChecksBetweenStartAndFinish()
{
if (IsPartialDefinition)
{
DeclaringCompilation.SymbolDeclaredEvent(this);
}
}

{
if (_getMethod is not null)
DeclaringCompilation.SymbolDeclaredEvent(_getMethod);

if (_setMethod is not null)
DeclaringCompilation.SymbolDeclaredEvent(_setMethod);
}

var completedOnThisThread = _state.NotePartComplete(CompletionPart.FinishPropertyParameters);
Debug.Assert(completedOnThisThread);
}
Expand Down
48 changes: 47 additions & 1 deletion src/Compilers/CSharp/Test/Emit2/Diagnostics/DiagnosticAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3231,7 +3231,7 @@ static partial void PartialMethod()
}

[Theory, CombinatorialData, WorkItem(32702, "https://github.com/dotnet/roslyn/issues/71149")]
public async Task TestPartialFileSymbolEndDiagnosticsAsync(bool separateFiles)
public async Task TestPartialMethodFileSymbolEndDiagnosticsAsync(bool separateFiles)
{
string definition1 = @"
internal partial class Test
Expand Down Expand Up @@ -3276,6 +3276,52 @@ internal partial class Test
);
}

[Theory, CombinatorialData, WorkItem(32702, "https://github.com/dotnet/roslyn/issues/71149")]
public async Task TestPartialPropertyFileSymbolEndDiagnosticsAsync(bool separateFiles)
{
string definition1 = @"
internal partial class Test
{
private partial object Prop { get; set; }
public Test(object _) { }
}";
string definition2 = @"
internal partial class Test
{
private partial object Prop { get => new(); set { } }
}";

string source1, source2;
if (separateFiles)
{
source1 = definition1;
source2 = definition2;
}
else
{
source1 = definition1 + definition2;
source2 = string.Empty;
}

var compilation = CreateCompilationWithMscorlib461([source1, source2]);
compilation.VerifyDiagnostics();

var tree1 = compilation.SyntaxTrees[0];
var semanticModel1 = compilation.GetSemanticModel(tree1);
var analyzers = ImmutableArray.Create<DiagnosticAnalyzer>(new SymbolStartAnalyzer(topLevelAction: false, SymbolKind.NamedType));
var compilationWithAnalyzers = compilation.WithAnalyzers(analyzers);

// Requesting diagnostics on a single tree should run the SymbolStart/End actions on all the partials across the compilation
// and the analysis result should contain the diagnostics reported at SymbolEnd action.
var analysisResult = await compilationWithAnalyzers.GetAnalysisResultAsync(semanticModel1, filterSpan: null, analyzers, CancellationToken.None);
Assert.Empty(analysisResult.SyntaxDiagnostics);
Assert.Empty(analysisResult.SemanticDiagnostics);
var compilationDiagnostics = analysisResult.CompilationDiagnostics[analyzers[0]];
compilationDiagnostics.Verify(
Diagnostic("SymbolStartRuleId").WithArguments("Test", "Analyzer1").WithLocation(1, 1)
);
}

[Fact, WorkItem(922802, "https://dev.azure.com/devdiv/DevDiv/_workitems/edit/922802")]
public async Task TestAnalysisScopeForGetAnalyzerSemanticDiagnosticsAsync()
{
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Test/Emit2/Diagnostics/GetDiagnosticsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,13 +286,15 @@ partial class Class
"",
"Class",
"DefOnlyPartialProp",
"get_DefOnlyPartialProp",
"get_ImplOnlyPartialProp",
"get_NonPartialProp1",
"get_PartialProp",
"ImplOnlyPartialProp",
"N1",
"NonPartialProp1",
"PartialProp",
"set_DefOnlyPartialProp",
"set_ImplOnlyPartialProp",
"set_NonPartialProp1",
"set_PartialProp"
Expand Down
11 changes: 4 additions & 7 deletions src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,13 +204,10 @@ void processMembers(IEnumerable<ISymbol> members)
memberSet ??= new HashSet<ISymbol>();
memberSet.Add(member);

// Ensure that we include symbols for both parts of partial methods.
// https://github.com/dotnet/roslyn/issues/73772: also cascade to partial property implementation part
if (member is IMethodSymbol method &&
!(method.PartialImplementationPart is null))
{
memberSet.Add(method.PartialImplementationPart);
}
if (member is IMethodSymbol { PartialImplementationPart: { } methodImplementation })
memberSet.Add(methodImplementation);
else if (member is IPropertySymbol { PartialImplementationPart: { } propertyImplementation })
Copy link
Member

Choose a reason for hiding this comment

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

Will we need to fix this up for field, with property initializers on the implementation part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this code would need to do anything differently based on the implementation part having an initializer. We also don't expect any backing field to be included in the set because IsImplicitlyDeclared will always be true on it.

memberSet.Add(propertyImplementation);
}

if (member is INamedTypeSymbol typeMember)
Expand Down
12 changes: 8 additions & 4 deletions src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,10 +1117,14 @@ static bool shouldIncludeSymbol(ISymbolInternal symbol, SyntaxTree tree, Cancell
return true;
}
}

// https://github.com/dotnet/roslyn/issues/73772: should we also check IPropertySymbol?
// there is no interface IPropertySymbolInternal
// where are tests for this?
else if (symbol is IPropertySymbolInternal propertySymbol)
{
if (propertySymbol.PartialDefinitionPart?.IsDefinedInSourceTree(tree, definedWithinSpan: null, cancellationToken) == true
|| propertySymbol.PartialImplementationPart?.IsDefinedInSourceTree(tree, definedWithinSpan: null, cancellationToken) == true)
{
return true;
}
}

return false;
}
Expand Down