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

Fix crash in speculative semantic model for local function attributes #41300

Merged
merged 5 commits into from
Feb 3, 2020
Merged

Fix crash in speculative semantic model for local function attributes #41300

merged 5 commits into from
Feb 3, 2020

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Jan 30, 2020 •
edited
Loading

Related to #38801
Related to (but does not fix) #24135

This fixes a crash when F5ing the local function attributes feature. The fix is based off a similar change made for type parameters.

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

Line 186 in 91571a3

binderOpt: (ContainingSymbol as LocalFunctionSymbol)?.SignatureBinder);

@RikkiGibson RikkiGibson requested a review from a team as a code owner January 30, 2020 00:31
agocke
agocke reviewed Jan 30, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs Outdated

var symbol = speculativeModel.GetDeclaredSymbol(localFunctionSyntax).GetSymbol<LocalFunctionSymbol>();
var attrs = symbol.GetAttributes();
Assert.Equal(new[] { "A" }, GetAttributeNames(attrs));
Copy link
Member

Choose a reason for hiding this comment

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

What's the underlying symbol here. Do we still have the type name/constructor name problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Will fix that separately.

src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs Outdated
var method = newTree.GetRoot().DescendantNodes().OfType<MethodDeclarationSyntax>().Single();
Assert.True(model.TryGetSpeculativeSemanticModelForMethodBody(method.Body.SpanStart, method, out var speculativeModel));

var symbol = speculativeModel.GetDeclaredSymbol(localFunctionSyntax).GetSymbol<LocalFunctionSymbol>();
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you ask the same thing using the regular semantic model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old model throws an exception if it is passed the new syntax node.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I meant if you constructed the final tree, then asked the question using a new semantic model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. I'll add it to the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@RikkiGibson RikkiGibson changed the title Fix speculative semantic model for local function attributes Fix crash in speculative semantic model for local function attributes Jan 30, 2020
@RikkiGibson RikkiGibson requested a review from a team January 31, 2020 00:07
cston
cston reviewed Jan 31, 2020
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs Outdated
@@ -264,11 +264,15 @@ private CustomAttributesBag<CSharpAttributeData> GetAttributesBag(ref CustomAttr
bagCreatedOnThisThread = LoadAndValidateAttributes(
this.GetReturnTypeAttributeDeclarations(),
ref lazyCustomAttributesBag,
symbolPart: AttributeLocation.Return);
symbolPart: AttributeLocation.Return,
binderOpt: (this as LocalFunctionSymbol)?.SignatureBinder);
Copy link
Member

@cston cston Jan 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

binderOpt: (this as LocalFunctionSymbol)?.SignatureBinder [](start = 20, length = 57)

Consider extracting a local:

else
{
    var binderOpt = ...;
    bagCreatedOnThisThread = forReturnType ?
        LoadAndValidateAttributes(…, binderOpt) :
        LoadAndValidateAttributes(…, binderOpt);
}

@RikkiGibson RikkiGibson requested a review from a team January 31, 2020 19:40
333fred
333fred reviewed Jan 31, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs Outdated
// Verify with speculative semantic model
var newTree = SyntaxFactory.ParseSyntaxTree(text + " ", TestOptions.RegularPreview);
var newMethod = newTree.GetRoot().DescendantNodes().OfType<MethodDeclarationSyntax>().Single();
Assert.True(model.TryGetSpeculativeSemanticModelForMethodBody(newMethod.Body.SpanStart, newMethod, out var speculativeModel));
Copy link
Member

Choose a reason for hiding this comment

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

Please actually insert new code in the attribute and use the speculative model to query info about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spoke last Friday about this. We agree that this test is using the speculative model to get a symbol for the attribute, correct?

333fred
333fred reviewed Jan 31, 2020
View reviewed changes
src/Compilers/CSharp/Test/Semantic/Semantics/LocalFunctionTests.cs Outdated
[Theory]
[InlineData("[A]")]
[InlineData("[return: A]")]
public void LocalFunctionAttribute_SpeculativeSemanticModel(string attributes)
Copy link
Member

Choose a reason for hiding this comment

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

Please test on parameters as well.

@333fred
Copy link
Member

333fred commented Jan 31, 2020

Done review pass (commit 4)

@RikkiGibson
Copy link
Contributor Author

I ended up just putting all 4 scenarios into a theory based on the existing test for type parameters. The new test wasn't doing anything meaningfully different from what Andy already wrote so.

@RikkiGibson
Copy link
Contributor Author

Had a test failure in release_32: TaggerProviderCreatedAfterInitialDiagnosticsReported

System.TimeoutException : Failed to clean up listeners in a timely manner.\r\n  EnqueueWorkItemAsync WorkCoordinator.SemanticChangeProcessor.cs 414\r\n  WorkItem WorkCoordinator.LowPriorityProcessor.cs 100\r\n---- System.OperationCanceledException : The operation was canceled.

   at Microsoft.CodeAnalysis.Test.Utilities.UseExportProviderAttribute.After(MethodInfo methodUnderTest) in /_/src/Workspaces/CoreTestUtilities/MEF/UseExportProviderAttribute.cs:line 126
----- Inner Stack Trace -----
   at System.Threading.CancellationToken.ThrowOperationCanceledException()
   at Roslyn.Test.Utilities.TaskJoinExtensions.JoinUsingDispatcherNoResult(Task task, CancellationToken cancellationToken)
   at Roslyn.Test.Utilities.TaskJoinExtensions.JoinUsingDispatcher(Task task, CancellationToken cancellationToken) in /_/src/Workspaces/CoreTestUtilities/TaskJoinExtensions.cs:line 22
   at Microsoft.CodeAnalysis.Test.Utilities.UseExportProviderAttribute.After(MethodInfo methodUnderTest) in /_/src/Workspaces/CoreTestUtilities/MEF/UseExportProviderAttribute.cs:line 94

I'm just going to rebase as I want a branch with these changes and the other recent local function changes to work off of.

@RikkiGibson RikkiGibson merged commit 2eb3bd5 into dotnet:features/local-function-attributes Feb 3, 2020
@RikkiGibson RikkiGibson deleted the lfa-speculative branch February 3, 2020 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@cston cston cston approved these changes

@agocke agocke Awaiting requested review from agocke

Assignees
No one assigned
Labels
Area-Compilers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@RikkiGibson @333fred @agocke @cston